Skip to content
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

Replace MAX_USER_MENTIONS with Settings::RateLimit.mention_creation #13736

Merged
merged 4 commits into from
May 12, 2021

Conversation

vaidehijoshi
Copy link
Contributor

@vaidehijoshi vaidehijoshi commented May 11, 2021

What type of PR is this? (check all applicable)

  • Optimization

Description

Addresses this TODO now that RFC #22 has been completed with #13367

# TODO: Vaidehi Joshi - Extract this into a constant or SiteConfig variable
# after https://github.com/forem/rfcs/pull/22 has been completed?
MAX_USER_MENTIONS = 7 # Explicitly set to 7 to accommodate DEV Top 7 Posts

Also removes my name from all the TODOs I added previously for myself 😿

Related Tickets & Documents

Follow up task to RFC #22

QA Instructions, Screenshots, Recordings

  1. Check that you cannot create a post with more than 7 @-mentions (this is the default value for Settings::RateLimit.mention_creation):

Screen Shot 2021-05-11 at 2 41 53 PM

  1. Check that you can create a post with 7 or fewer @-mentions:

Screen Shot 2021-05-11 at 2 42 01 PM

  1. Check that you cannot create a comment with more than 7 @-mentions

Screen Shot 2021-05-11 at 2 42 22 PM

  1. Check that you can create a comment with 7 or fewer @-mentions:

Screen Shot 2021-05-11 at 2 42 37 PM

  1. Go to the admin view /admin/customization/config, and check that you can see the limit for @-mentions in a post or comment in the "rate limit and antispam" section:

Screen Shot 2021-05-11 at 2 43 39 PM

  1. Try changing that value and updating the config:

Screen Shot 2021-05-11 at 2 44 06 PM

You should now only be able to create up to the new number of max mentions that you just set 😃

UI accessibility concerns?

A backend change, so...none!

Added tests?

  • Yes

[Forem core team only] How will this change be communicated?

  • I've updated the README or added inline documentation
  • I will share this change in a Changelog
    or in a forem.dev post
  • I will share this change internally with the appropriate teams

Are there any post deployment tasks we need to perform?

I don't think we have any; Settings::RateLimits are already in production, so everything should Just Work™️

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label May 11, 2021
@vaidehijoshi vaidehijoshi requested review from a team, joshpuetz, juliannatetreault and citizen428 and removed request for a team May 11, 2021 22:00
Copy link
Contributor

@juliannatetreault juliannatetreault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works like a charm locally! ✨

The only thing that stood out to me was the when I add @-mentions to posts, my editor seems to lag out--I'm only able to add a single character at a time when typing out a username and have to manually "refocus" my cursor within the editor to finish the @-mention. I tested this on main and experienced the same thing, so it doesn't seem like this is unique to your work (and might just be a "me" thing for all I know!), but I wanted to mention it just in case!

Also, 😿 at the removal of your name within the codebase.

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels May 11, 2021
@vaidehijoshi
Copy link
Contributor Author

The only thing that stood out to me was the when I add @-mentions to posts, my editor seems to lag out--I'm only able to add a single character at a time when typing out a username and have to manually "refocus" my cursor within the editor to finish the @-mention. I tested this on main and experienced the same thing, so it doesn't seem like this is unique to your work (and might just be a "me" thing for all I know!), but I wanted to mention it just in case!

Interesting...I tried to repro this locally but my local editor's autocomplete functionality seems to be pretty snappy 🤔 If you're still seeing this locally (on main and after restarting your browser), then it might be worth opening up an issue for it! :)

@juliannatetreault
Copy link
Contributor

The only thing that stood out to me was the when I add @-mentions to posts, my editor seems to lag out--I'm only able to add a single character at a time when typing out a username and have to manually "refocus" my cursor within the editor to finish the @-mention. I tested this on main and experienced the same thing, so it doesn't seem like this is unique to your work (and might just be a "me" thing for all I know!), but I wanted to mention it just in case!

Interesting...I tried to repro this locally but my local editor's autocomplete functionality seems to be pretty snappy 🤔 If you're still seeing this locally (on main and after restarting your browser), then it might be worth opening up an issue for it! :)

Okay... definitely sounds like a "me" thing then! I haven't restarted my browser yet, but I will definitely open an issue for it if it persists after restarting it. 😊

@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels May 12, 2021
@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels May 12, 2021
@vaidehijoshi
Copy link
Contributor Author

Ahead of merging this PR, I've updated the Forem Admin Guide to include this new configuration! ✅

@vaidehijoshi vaidehijoshi merged commit 5e6aad9 into main May 12, 2021
@vaidehijoshi vaidehijoshi deleted the vaidehijoshi/extract-max-user-mentions-to-variable branch May 12, 2021 15:16
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels May 12, 2021
@vaidehijoshi vaidehijoshi added the changelog: rollup Items that will be communicated in our monthly rollup changelogs label May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: rollup Items that will be communicated in our monthly rollup changelogs PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants