FEATURE: Add option to grant badge multiple times to users using Bulk Award (PR #13571)

Currently when bulk-awarding a badge that can be granted multiple times, users that appear multiple times in the CSV file are granted the badge only once if they have not been granted the badge before, and never if they already have the badge.

This PR adds a new option to the Badge Bulk Award feature so that it’s possible to grant users a badge even if they already have the badge and as many times as they appear in the CSV file:

The way this feature works is when you upload a CSV file, the Admin::BadgesController#mass_award action looks up the current sequence numbers of the badge for all the users in the CSV file, and then passes this data to the instances of MassAwardBadge it enqueues.

The rationale behind making the controller action look up the current sequence numbers instead of the MassAwardBadge job is to make the job “crash safe”. I.e., if the job looked up the current sequence numbers and it crashed midway through, Sidekiq would retry the job later and the job could potentially see new sequence numbers in the database and grant some users the badge more times than it should. In the current design we have no such problem because the controller passes the sequence numbers to the job and in the event it’s retried it reuses the same sequence numbers.

GitHub

Something doesn’t feel right here. Why do we have to target the parent element? Can we add a class to the element so that we can click it directly?

Ditto: FEATURE: Add option to grant badge multiple times to users using Bulk Award by OsamaSayegh · Pull Request #13571 · discourse/discourse · GitHub

The description needs to be updated here.

    if grant_existing_holders && !badge.multiple_grant?

A bit of a distraction/suggestion, I feel like this controller action is getting too big. Perhaps it is time to split the logic here into a service object?

          user_with_badge = Fabricate(:user).tap { |u| BadgeGranter.grant(badge, user_with_badge) }

I don’t feel like these assertions are necessary as long as we’re certain that we’ve fabricated correctly

Now that I’ve reduced the new logic that I need to add in the controller action to roughly half what it was before, do you still feel we should still reduce the controller action and extract things into a service object?

@tgxworld @SamSaffron I’ve updated the PR so now we have new class method mass_grant_existing_holders on BadgeGranter that the MassAwardBadge jobs calls when the new checkbox (shown in the screenshot in post #1) is ticked when bulk-awarding a badge to users. Each time a file is uploaded, we assign a unique ID to group all the jobs/batches that will be enqueued and each job/batch is assigned a number.

Then when it’s time to process a batch, the mass_grant_existing_holders checks in redis if this batch has bee processed before, and if it has been then it skips granting the badge to the users in the batch, but if it has not then it goes ahead and grants the badge to the users in a transaction and before the transaction finishes it stores in redis that this batch has been processed (and also stores the IDs of users in the batch in a redis set).

When the last batch finishes processing, it fetches the IDs of users of all the other batches from redis, and creates a notification for each user and updates various stats.

Thoughts on this?

I’m wonder if we’ll run into styling problems if both label and input has the same class.

If the badge is nil or not enabled, I think we can return much earlier in the job instead of after fetching the users.

FWIW you could have an @action here, and use it directly in form instead of having onFileInputChange

should be wrapped un a span

I’m not sure 3px is something we use a lot ? could we use 5px or 0.25em ?

maybe use .with_index here, so we don’t have all the line_number code ?

I think we’ve got to look up the data from the DB in batches here. Plucking 50,000 records in one go is quite alot.

This doesn’t feel right to me. Both variables are assigned to the same object. If the CSV has a mix of usernames and emails, we’ll actually discard all the usernames.

Hmm I think we need to be careful here. If they accidentally upload a wrong CSV in the right format, unmatched might be an array containing 50K items.

I think we need to use sucess_json here since there is a field that the ajax library on the client side expects.