Quoting broken when name matches username

Continuing from Configuring Twitter login (and rich embeds) for Discourse - #114 by Hifihedgehog

I got an email about this and was very confused as I didn’t recall making a post like this.

I’m curious how the “taylor” got swapped here in the quote reply? Is this some manual edit mistake (I’m not sure how to do a quote block with a source manually, or if that is even possible). Or is there a bug somewhere in the reply logic? @tshenry

1 Like

I’ve just tried quoting a few posts, and I think something funky is going on with username vs fullname. Probably this recent commit:

1 Like

Hi @JammyDodger,

can you give me some more info into

With the site settings of prioritize username in ux turned on and display name on posts turned off you should see the username in the quote, and vice versa for seeing the full name.

1 Like

Sorry, I could have been much more helpful there. :slightly_smiling_face: I just tried quoting several of the posts in this topic, and there were some odd results in my composer:

@Hifihedgehog’s post:

[quote=", post:114, topic:13395"]
you right! That took all of a couple minutes and I was instantly approved. Simple fixes are always nice! :smile:
[/quote]

@tshenry’s post:

[quote="Taylor, post:113, topic:13395"]
You can gain access to the v1.1 API for free under the Elevated plan. You simply need to apply from your developer dashboard. I found it very quick and easy. Discourse Twitter logins worked flawlessly after that :tada:
[/quote]

(that’s the style of one @taylor got a notification for)

4 Likes

Ah, thanks for pointing this out… yah, this is not desirable / intended. I will look into this!

3 Likes

So the reason @taylor got tagged in this quote (and got an email) is because his username - Taylor is the same as @tshenry’s fullname - Taylor :open_mouth: which (the fullname) is being prioritized properly in quotes because of the changes added in DEV: Prioritize full name when setting active (#15820) · discourse/discourse@5a93ce4 · GitHub.

Anyways, thanks for tip, I’ll get this fixed!

9 Likes

FWIW, It also leaves quotes without fullnames blank:

4 Likes

Are we good on this now @isaac?

Yes we reverted the change, and I am working on fixing logic before merging again

3 Likes

In updating the quote logic to allow for us to toggle username or full name via a site setting I have run into a problem (part of the problem was noted above), let me create a hypothetical scenario…

  • Tim creates a post
  • Site Setting is added and activated to toggle the OP display name to be username (was active) or full name (now active)
  • A user quotes Tim in a new post, but because we are now using the full name for quotes, our other user TimTam, full name: Tim, gets notified that someone quoted him on a post he never created.

Rebaking all of the quotes is also a no go. If this site setting was toggled multiple times, and we rebake multiple times, a quote that is being converted from a full nameusername is not unique and could find multiple users.

The avenue I see us taking is adding an additional data attribute to the quote of data-full-name="true" (or something) to check if we should replace the username with the full name in the ux, instead of manipulating the data-username

eg.

data-username="always.users.username" data-post="1" data-topic="1" data-full-name="true"

and save ourselves the hassle of worrying if we are querying or sending notifications on username or full name. Thoughts? Is this worth the effort of updating PrettyText and our quote builder for this change?

5 Likes

I prefer not to add support for a switchover. This is a decision you make early in your community setup, supporting super smooth switchover is extremely expensive and comes with a bunch of trade-offs.

If I quote you with username prioritized we render this markdown

[quote="isaacjanzen, post:12, topic:217633"]
Thoughts?
[/quote]

If i quote you with name prioritized we render this markdown

[quote="Isaac Janzen, post:12, topic:217633"]
Thoughts?
[/quote]

Got to be careful with , in full names - but I support stripping them out for this use case or I guess you can introduce some escaping trick.

4 Likes

Are you saying you would recommend we scrap trying to add the ability to display quotes with full names? We are inevitably going to run into these switchover cases… eg. any existing community with a quote

1 Like

Nope, not fully scratch it… meta has prioritize username in ux disabled. The quote I just made should be: (which is missing an avatar which needs fixing)

The markup here should be:

[quote="Isaac Janzen, post:14, topic:217633"]
Are you saying you would recommend we scrap trying to add the ability to display quotes with full names?
[/quote]

Only change needed it to make the quote widget be aware of the setting so it fills in full name instead of username. Do not worry about migration and history.

1 Like

I am not sure we are on the same page here… when we merged DEV: Prioritize full name when setting active (#15820) · discourse/discourse@5a93ce4 · GitHub we achieved

  # quote.js
  # opts.displayName = true if site settings prioritize full name
  const name = opts.displayName
    ? opts.name || post.name
    : opts.username || post.username;

  # build quote markdown
  const params = [
    name,
    `post:${opts.post || post.post_number}`,
    `topic:${opts.topic || post.topic_id}`,
  ];

to get the output of

but the problem was that the user lookup based on full name was breaking existing quotes that were baked with username.

So it feels that it is impossible to ignore

1 Like

I am super confused cause meta has:

So why is the name missing from the quote above? Was the change rolled back?

Agree we need to do more here:

We need to grab the username from “post / topic” combination and place it in the cooked markdown (we should do that unconditionally), otherwise we can not display avatars for the full name.

It also lets us fix this edge case, so it does not show my avatar on this mis-quote. (or even highlights a misquote)

1 Like

Ah, yah

I agree, and can add this.

2 Likes

I am totally open to fix a few edge cases while there:

  1. username mismatch
    • what do we do about avatar?
    • what do we do about username?
  2. full name mismatch
    • what do we do about avatar?
    • what do we do about name?

I think the best thing to do for now is just “override”. If the post has a name / username, use it over the one that was supplied in the quote. Cook in current name / current username.

3 Likes

Alright, I have a PR up that clears things up.

Key takeaways

We now pass the username when

siteSettings.display_name_on_posts && !siteSettings.prioritize_username_in_ux && post.name

as well as the full name to guarantee that we are not getting any mismatches when querying for user / avatar.

eg.

[quote="Isaac Janzen, post:3, topic:7, full:true, username:isaac.janzen"]
bing bong
[/quote]

What are your thoughts @sam?

3 Likes

I guess I can accept a markdown change so we work around needing to go into a rabbit hole of an enormous security adventure. However the change needs to be hyper surgical.

Can you confirm that the new verbose syntax only ever happens if both siteSettings.display_name_on_posts && !siteSettings.prioritize_username_in_ux ?

[quote="Isaac Janzen, post:3, topic:7, username:isaac.janzen"]
....
[/quote]

Meaning that if you do not enable both display_name_on_posts AND disable prioritize_username_in_ux

Then the old quote format of should remain:

[quote="isaac.janzen, post:3, topic:7"]
....
[/quote]

I am willing to accept this as an intermediate step for now, but our long term goal would be to remove this noise from the quote block (tighten security, restrict abuse) and support the abridged quote format we always had, working through the security can of snakes, just like we are forced to now for inline oneboxes eg: Quoting broken when name matches username - #20 by isaachttps://meta.discourse.org/t/quoting-broken-when-name-matches-username/217633/20?u=sam

Mentioning @tobiaseigen here for visibility cause this relates to previous discussions.

Bottom line:

  • IMO OK to change markdown format for now for this edge case - and only this edge case - @codinghorror to confirm.
  • TBD schedule security work so quote block becomes [quote="TOTALLY OPTIONAL WILL BE FISHED FROM POST IF SECURITY LINES UP AND INFO ON POST IS PRIORITIZED OVER THIS TEXT, post:3, topic:7"]. - recommend we wait a while on this.
3 Likes

Merged and deployed DEV: Prioritize full name when display_name_on_posts active by janzenisaac · Pull Request #16078 · discourse/discourse · GitHub

I can confirm.

3 Likes