Quoting broken when name matches username

[JammyDodger]

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

DEV: Prioritize full name when setting active (#15820)

[isaacjanzen]

Hi @JammyDodger,

can you give me some more info into

something funky is going on

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.

[JammyDodger]

Sorry, I could have been much more helpful there. 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)

[isaacjanzen]

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

[isaacjanzen]

@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]

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 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!

[JammyDodger]

FWIW, It also leaves quotes without fullnames blank:

you right! That took all of a couple minutes and I was instantly approved. Simple fixes are always nice!

[codinghorror (Jeff Atwood)]

Are we good on this now @isaacjanzen?

[isaacjanzen]

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

[isaacjanzen]

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 name → username 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?

[sam (Sam Saffron)]

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.

[isaacjanzen]

@sam

I prefer not to add support for a switchover. This is a decision you make early in your community setup

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

[sam (Sam Saffron)]

@isaacjanzen

Are you saying you would recommend we scrap trying to add the ability to display quotes with full names?

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)

@Isaac Janzen

Are you saying you would recommend we scrap trying to add the ability to display quotes with full names?

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.

[isaacjanzen]

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

@sam

make the quote widget be aware of the setting so it fills in full name instead of username

  # 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

@sam

[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]

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

@sam

Do not worry about migration and history.

[sam (Sam Saffron)]

@isaacjanzen

I am not sure we are on the same page here

I am super confused cause meta has:

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

@isaacjanzen

So it feels that it is impossible to ignore

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)

@sam

So it feels that it is impossible to ignore

[isaacjanzen]

Ah, yah

@isaacjanzen

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

@sam

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.

I agree, and can add this.

[sam (Sam Saffron)]

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.

[isaacjanzen]

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?

[sam (Sam Saffron)]

@isaacjanzen

We now pass the username

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 isaacjanzen ← https://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.

[isaacjanzen]

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

@sam

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

@sam

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

I can confirm.

[sam (Sam Saffron)]

@isaacjanzen

I can confirm.

Thanks!

I am still a bit mixed on adding , username: isaac.jenzen in the quote block cause we can get it all directly from the topic / post. But given the defaults are not changed I think I am OK with this for now.