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

      DB.exec(<<~SQL * count, now: Time.zone.now, system: Discourse.system_user.id, user_id: user.id, badge_id: badge.id)
            WHERE badge_id = :badge_id AND user_id = :user_id
      expect(instances.map(&:notification_id).uniq).to contain_exactly(notifications.first.id)

I also think we should test for the sequences to make sure the seq has been set correctly.

We could do that, but we’d still need to keep line_variable variable because if we encounter an error while reading the CSV file, we report the problematic line number to the user (see line 131 in the controller).

Let’s test for the response code here.

I tried 5px initially, but I felt like the icon and text were little too far apart so I reduced to 3. But happy to change it if you feel strongly about it.

This is how it looks now:

If we’re already stubbing the constant, let’s stub it to a small number to avoid allocating a larger than required string.

          expect(response.parsed_body['unmatched_entries']).to contain_exactly(
            "nonexistentuser",
            "nonexistentemail@discourse.fake"
          )
            expect(notifications.map(&:notification_type).uniq).to contain_exactly(Notification.types[:granted_badge])

I feel like alot of the test cases here should be moved under the BadgeGranter service object spec. For controller tests, I usually prefer to test the various response code that we expect. Success, errors etc.

Can this block of code be done while we’re reading the CSV file? This will eliminate one extra loop through a potentially large array.

Right, they do share the same object here, but later in the lines directly after this one the variables are reassigned to different objects so if there are emails or usernames, they’ll end up referencing different objects. I did this to make the code 1 line shorter, but I guess it’s not really worth it :sweat_smile: Will split this into 2 lines.

Hmm I’m wondering if we actually need to do this here. If we don’t allow a mix of usernames and emails in the file, we can just key the count_per_user on the email or username. The sidekiq job already has to lookup the user again so we might as well just pass it an email or username.

@OsamaSayegh Some comments but I 100% this approach is much better given that it is way easier to understand what is going on.

I’m not sure what you mean here; success_json is simply this:

Do you mean I should merge my hash with success_json?

I’ve moved most of the logic in Admin::BadgesController#mass_award into a new static method in BadgeGranter and moved some of the tests here to BadgeGranter spec file. Let me know if I should move more.

I faintly recall that somewhere on the client side looks for the success field but I can’t find it anymore. Let’s ignore my comment here :slight_smile:

Looks good to me :+1: Apologies for the multiple iterations that this had to go through.

Thanks TGX, I appreciate the thorough review, the PR now is in a much better shape than it was before your feedback!