New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FEATURE: Add an emoji deny list site setting #20929
Conversation
12b26a3
to
b665dbd
Compare
The site setting has no default default values selected in this pr (blank string). This means that adding this feature should not affect any emojis when merged in. The denied emojis will only kick in once a user adds the emojis via the site settings interface. |
app/assets/javascripts/discourse/app/components/emoji-picker.js
Outdated
Show resolved
Hide resolved
app/assets/javascripts/pretty-text/engines/discourse-markdown/emoji.js
Outdated
Show resolved
Hide resolved
23c07b4
to
4504920
Compare
4504920
to
e8da68a
Compare
Added a new model method at |
@dbattersby Code is looking much simpler to me 👍 I do have some concerns about performance and some minor comments but the code looks good to me otherwise. |
46c6de8
to
7453538
Compare
7453538
to
054ffbc
Compare
app/models/emoji.rb
Outdated
if denied_emojis.present? | ||
all.reject { |e| denied_emojis.include?(e.name) } | ||
else | ||
all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're kind of double caching here since Emoji.all
is already cached. This ends up adding more data to our cache unnecessarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually.... this also means that if the number of denied emojis is low, we also end up double caching alot of the data 🤔
[5] pry(main)> now = Time.zone.now; Emoji.all.reject { |emoji| ["alien", "robot"].include?(emoji.name) }; Time.zone.now - now
=> 0.003174472
Looping through all the emojis add 3ms to each page load though 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange, but I really can't see any difference in the response time between calling .all
and all.reject
. However Emoji.denied
is much faster:
[4] pry(main)> now = Time.zone.now; Emoji.all.reject { |emoji| ["alien", "robot"].include?(emoji.name) }; Time.zone.now - now
=> 0.006026
[5] pry(main)> now = Time.zone.now; Emoji.all; Time.zone.now - now
=> 0.006118
[6] pry(main)> now = Time.zone.now; Emoji.denied; Time.zone.now - now
=> 0.001315
[7] pry(main)> now = Time.zone.now; Emoji.denied; Time.zone.now - now
=> 0.000592
In the above example there are 4 denied emoji in site settings. On the first call the Emoji.denied
relies on .aliases
which reads from a json file on first try, then subsequent calls will read from the cache. On second request it drops from 1ms to 0.5ms.
My personal opinion is that even at 3ms it is negligible, but I'm open to ideas here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best solution here might be to remove the Emoji.allowed
cache since .all
is already cached?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea sorry for the confusion, let's just remove the cache for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think I found a better way for this to prevent the double caching:
def self.load_allowed
denied_emojis = denied
all_emojis = load_standard + load_custom
if denied_emojis.present?
all_emojis.reject { |e| denied_emojis.include?(e.name) }
else
all_emojis
end
end
By using load_standard + load_custom, we can get all the default and custom user emojis. Then remove denied emojis and cache the result.
[1] pry(main)> Emoji.clear_cache
=> {}
[2] pry(main)> now = Time.zone.now; Emoji.all; Time.zone.now - now
CustomEmoji Load (1.6ms) SELECT "custom_emojis".* FROM "custom_emojis" ORDER BY "custom_emojis"."name" ASC
=> 0.05434
[3] pry(main)> now = Time.zone.now; Emoji.all; Time.zone.now - now
=> 0.006179
[4] pry(main)> now = Time.zone.now; Emoji.allowed; Time.zone.now - now
CustomEmoji Load (1.3ms) SELECT "custom_emojis".* FROM "custom_emojis" ORDER BY "custom_emojis"."name" ASC
=> 0.023715
[5] pry(main)> now = Time.zone.now; Emoji.allowed; Time.zone.now - now
=> 0.006197
Now at just over 2ms on first load then down to 0.6ms when reading from the cache.
…ue list, filter chat emoji favorites
… to work with emoji cache
34ac51f
to
6a1d728
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Do remember to squash and update the commit title/description when merging. Thank you!
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/support-more-management-to-emoji-reaction-in-chat/249001/14 |
This pull request has been mentioned on Discourse Meta. There might be relevant details there: |
This feature will allow sites to define which emoji are not allowed. Emoji in this list should be excluded from the set we show in the core emoji picker used in the composer for posts when emoji are enabled. And they should not be allowed to be chosen to be added to messages or as reactions in chat. This feature prevents denied emoji from appearing in the following scenarios: - topic title and page title - private messages (topic title and body) - inserting emojis into a chat - reacting to chat messages - using the emoji picker (composer, user status etc) - using search within emoji picker It also takes into account the various ways that emojis can be accessed, such as: - emoji autocomplete suggestions - emoji favourites (auto populates when adding to emoji deny list for example) - emoji inline translations - emoji skintones (ie. for certain hand gestures)
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/trying-to-prevent-some-emojies/272559/2 |
This feature will allow sites to define which emoji are not allowed. Emoji in this list should be excluded from the set we show in the core emoji picker used in the composer for posts when emoji are enabled. And they should not be allowed to be chosen to be added to messages or as reactions in chat. This feature prevents denied emoji from appearing in the following scenarios: - topic title and page title - private messages (topic title and body) - inserting emojis into a chat - reacting to chat messages - using the emoji picker (composer, user status etc) - using search within emoji picker It also takes into account the various ways that emojis can be accessed, such as: - emoji autocomplete suggestions - emoji favourites (auto populates when adding to emoji deny list for example) - emoji inline translations - emoji skintones (ie. for certain hand gestures)
This feature will allow sites to define which emoji are not allowed. Emoji in this list should be excluded from the set we show in the core emoji picker used in the composer for posts when emoji are enabled. And they should not be allowed to be chosen to be added to messages or as reactions in chat.
This feature prevents denied emoji from appearing in the following scenarios:
It also takes into account the various ways that emojis can be accessed, such as:
/t/93605/