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

FIX: Badge and user title interaction fixes #8282

Merged

Conversation

@martin-brennan
Copy link
Contributor

martin-brennan commented Nov 1, 2019

Original meta thread: https://meta.discourse.org/t/engagement-level-x-badge-nucommunity/129514
Dev thread: https://dev.discourse.org/t/badge-titles-not-revoked-when-badge-name-is-customized/16978

The issue described here is that BadgeGranter#revoke_ungranted_titles! was not taking into account system badges with custom text set via TranslationOverride, and thus when a badge was revoked that was being used for a user title, the user still retained that title. There are a few different fixes in this PR:

  • When the user title is changed (manually or setting it via using the badge_title route in UsersController), we now consider badge_granted_title to be true on UserProfile if the title matches either the badge name OR the custom TranslationOverride text for the badge
  • Introduces a new granted_title_badge_id column to UserProfile which is set at the same time as above, so we have a record of which badge granted the user title
  • Added a migration to backfill granted_title_badge_id for all users who have a title based on a badge name OR based on translation override text for a badge
  • When a badge name is changed, or when custom text is added/revoked for a system badge, we find all user profiles using that badge for a title via granted_title_badge_id and update the user's title to match the new text
  • When a badge is revoked if the user title matches the badge name OR custom text then we revoke the user's title
  • Finally, added a change_title and a revoke_title UserHistory action and corresponding staff log, to keep better track of when and where title changes occur

Notes

  • I did intend to split the code into BulkUserTitleUpdater into a background job, but I'm now leaning towards saying its probably going to be unlikely that these will be hugely slow queries to run. Thoughts?
  • The BulkUserTitleUpdater does not add individual UserHistory records for each change_title action it carries out, I'm not sure if this is a problem though
  • The migration has only been tested on a very small amount of users locally
* fix an issue where a user's title was not considered
  a badge granted title when the user used a badge for
  their title and the badge name was customized. this
  affected the effectiveness of revoke_ungranted_titles!
  which only operates on badge_granted_titles
* when a user's title is set now it is considered a
  badge_granted_title if the badge name OR the badge
  custom name from TranslationOverride is the same as
  the title
* when a user's badge is revoked we now also revoke their
  title if the user's title matches the badge name OR
  the badge custom name from TranslationOverride
* add a user history log when the title is revoked to remove
  confusion about why titles are revoked
* now when we set badge_granted_title on a user profile
  when updating a user's title based on a badge, we also
  remmeber which badge matched the title
* this will make it easier to then find all users which
  are using a badge for their title when a badge's name
  text changes, so titles can also be updated
* add more logging around user title changes as well
* when the name of a badge changes, or in the case of system
  badges when their custom translation text changes, then we
  need to update the title of all corresponding users who
  have a badge_granted_title and matching granted_title_badge_id
* in the case of system badges we need to first get the proper
  badge ID based on the translation key e.g. badges.regular.name
@discoursebot

This comment has been minimized.

Copy link

discoursebot commented Nov 1, 2019

You've signed the CLA, martin-brennan. Thank you! This pull request is ready for review.

* for both normal badge name titles and titles using custom
  badge text
@martin-brennan martin-brennan changed the title [WIP] FIX: Badge and user title interaction fixes FIX: Badge and user title interaction fixes Nov 1, 2019
* this is not set for custom badge text titles, which is the
  initial problem!
@martin-brennan martin-brennan requested a review from SamSaffron Nov 1, 2019
@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Nov 6, 2019

@vinothkannans would you mind reviewing this?

new_value: user.title
}

if current_user.staff?

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

I think we shouldn't add a staff action log for a staff's own title change. Maybe it should be like if current_user != user?

previous_value: previous_title
}

if current_user.staff?

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

Same here. It should be if current_user != user.

SET granted_title_badge_id = b.id
FROM users
INNER JOIN badges b ON users.title = b.name
WHERE users.title = b.name

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

Why we checking the users.title = b.name both in inner join condition and where condition?

WHERE users.title = b.name
AND user_profiles.granted_title_badge_id IS NULL
AND users.id = user_profiles.user_id
AND users.title IS NOT NULL AND users.title != ''

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

I think we don't have to check the users.title here again, since we already checked it like users.title = b.name. If it's empty it won't even satisfy the existing inner join condition.

@@ -1997,12 +1997,35 @@ def filter_by(method)
expect(user.user_profile.reload.badge_granted_title).to eq(false)

badge.update!(allow_title: true)
user.badges.reload

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

We don't need to reload the badges here. Since we only set the title of user.

This comment has been minimized.

Copy link
@martin-brennan

martin-brennan Nov 6, 2019

Author Contributor

I do need to do that because the spec fails without doing so. The User#check_if_title_is_badged_granted method uses ruby Enumerable#find to save trip to the DB https://devdocs.io/ruby~2.6/enumerable#method-i-find.

badge_matching_title = title.present? && badges.find do |badge|
  badge.allow_title? && (badge.display_name == title || badge.name == title)
end

Because the user's badges AR association is already loaded it will not query the DB again to fetch the updated allow_title. I can alternatively call user.badges.find { |b| b.id == badge.id }.update!(allow_title: true) but I think that's much of a muchness really. Or I can change the User model code to fetch the badges from the db instead of using badges.find. Is there a downside to reloading that I'm not aware of?

This comment has been minimized.

Copy link
@martin-brennan
Copy link
Member

vinothkannans left a comment

I did intend to split the code into BulkUserTitleUpdater into a background job, but I'm now leaning towards saying its probably going to be unlikely that these will be hugely slow queries to run.

I think it's okay to put it in a background job. Since only the limited number of users who use a badge's name as title is going to be updated.

@@ -0,0 +1,36 @@
# frozen_string_literal: true
class BackfillGrantedTitleBadgeIdForUserProfile < ActiveRecord::Migration[6.0]

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 6, 2019

Member

Instead of creating a separate migration class, we can add the backfill in AddGrantedTitleBadgeIdToUserProfile class itself like this.

This comment has been minimized.

Copy link
@martin-brennan

martin-brennan Nov 6, 2019

Author Contributor

No problem, will do that.

@martin-brennan

This comment has been minimized.

Copy link
Contributor Author

martin-brennan commented Nov 6, 2019

@vinothkannans thanks for the review. For this last point:

I think it's okay to put it in a background job. Since only the limited number of users who use a badge's name as title is going to be updated.

Did you mean to say it's okay to NOT put it in a background job? If you meant for me to move it into a background job that's fine and I will do so :)

@martin-brennan

This comment has been minimized.

Copy link
Contributor Author

martin-brennan commented Nov 7, 2019

@vinothkannans made all the changes except for my comments left about the background job & reloading user badges.

@martin-brennan martin-brennan requested a review from vinothkannans Nov 7, 2019
@vinothkannans

This comment has been minimized.

Copy link
Member

vinothkannans commented Nov 7, 2019

Did you mean to say it's okay to NOT put it in a background job?

No, I said that we can put it in a background job. @martin-brennan

@martin-brennan

This comment has been minimized.

Copy link
Contributor Author

martin-brennan commented Nov 7, 2019

Did you mean to say it's okay to NOT put it in a background job?

No, I said that we can put it in a background job. @martin-brennan

No problem, sorry I just read your initial comment incorrectly and confused myself! 😅

user_has_badge_like_title = badge_matching_title.present?
user_profile.update(
badge_granted_title: user_has_badge_like_title,
granted_title_badge_id: user_has_badge_like_title ? badge_matching_title.id : nil

This comment has been minimized.

Copy link
@vinothkannans

vinothkannans Nov 7, 2019

Member

Super minor but you can do badge_matching_title&.id here instead of inline if.

This comment has been minimized.

Copy link
@martin-brennan

martin-brennan Nov 7, 2019

Author Contributor

No problem. We had banned the safe navigation operator at my old job so I need to get myself in the habit of using it

* add specs for job
* change controller for badges + site texts to call
  the job instead of the old service for title updating
@martin-brennan

This comment has been minimized.

Copy link
Contributor Author

martin-brennan commented Nov 8, 2019

@vinothkannans have moved the updater into a job now.

@martin-brennan martin-brennan requested a review from vinothkannans Nov 8, 2019
Copy link
Member

vinothkannans left a comment

LGTM 👍

@martin-brennan martin-brennan merged commit 56d3e29 into master Nov 8, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@martin-brennan martin-brennan deleted the fix/badge-title-revoke-when-badge-name-customized branch Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.