FIX: problems with choosing favorite badges (PR #13731)

Choosing favorite badges started behaving strange recently.

This PR fixes the problem.

I’m a bit concerned about API method, and I’d like to change it in subsequent PR if there are no objections. We have this method:

PUT http://localhost:4200/user_badges/20/toggle_favorite

And here is JSON this method returns:

It’s quite a lot. In fact, this method could be returning only status code. That would be enough to figure out everything on the client side. No need to serialize objects, no need to send them to the client. It’ll be a bit more efficient, but I’m also concerned about clearness of API. Looks like there’s no need to return anything from this method.

Even more, instead of toggle_favorite it can be idempotent method that sets is_favorite to provided value, and returns only status code as well

PUT http://localhost:4200/user_badges/20/favorite
{ true }

or
PUT http://localhost:4200/user_badges/20/favorite
{ false }

GitHub

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

It’s always smart to select the values you want to update by adding where(is_favorite: !new_is_favorite_value). It seems counter intuitive but it performs better in psql.

Sorry not related to your change but while we’re here can we handle the possible failure mode here? update can fail and we’re currently ignoring that case here.

      expect(parsed["user_badge"]["is_favorite"]).to eq(true)

Let’s follow this syntax since the surround code is already doing so. Feels weird to have 2 different syntax within the same file.

Also it looks like running UserBadge.where().update() ends up running an update query per UserBadge that is returned from the filter. Perhaps we can switch this to use update_all?

Looks good to me. I agree with you that we should try and move to an API where the desired state is passed as the param.

@tgxworld We’ve just discussed with Régis in chat that it’s better to stick with the toggle pattern since we use it in other places :slight_smile: I’ll remove returning unnecessary data to the client in subsequent PR though.

I think we can probably leave toggle here since it’s already implemented in this way. But I’d discuss it in the future again, since idempotent PUT is much more predictable and robust.

It’s a fair point :+1:

A nice suggestion about update_all, it does one UPDATE request instead of separate requests for every badge.

can we handle the possible failure mode here? update can fail and we’re currently ignoring that case here.

Not sure if I follow here. As I understand - the only failure we can get here is some connection failure or something else of this kind. It’s impossible that this UPDATE fail because of a mistake in business logic, there are no constraints on the DB level that have something to do with favorite badges.

Looks like we shouldn’t handle failures here :thinking:

Previously with update(), active record callbacks will run. Even though we might not have callbacks now, someone in the future might come along and add a validation which may cause the update to fail.

Hmm, a good point in general, but I’m afraid in this specific case it can cause false assumptions that a part of users’ badges of specific type can be marked as favorite and a part is not. Which isn’t the case, we always toggle all users’ badges of specific type.

Oh, I see. But I don’t see examples of such checks in our controllers. This kind of validation can be pretty unobvious. For example, in this method we do all validation before calling update which is explicit and perfectly fine in my opinion.

Frankly I think if someone for some reason decide to do validation in the update hook he should take care of handling this at that moment. If he forgets to handle it, rails will generate and return 500 error to him when he’ll be testing his new validation.

The problem with not checking for the return value of update() is that the failure becomes silent. A person not working on this method in the controller can add a validation else where for a different purpose. As you’ve said though, the chances of failure is low but the amount of code we have to write to handle failure is so little that it is worth it anyway. We’ve updated the code to use update_all() so the comments here are no longer relevant though.

Oh, I get it now, thank you :slight_smile: update_all doesn’t instantiate models and doesn’t trigger ActiveRecord callbacks.

I don’t quite understand this, could you expand and perhaps provide an example? I am suggesting the query should go from something like this:

UPDATE user_badges SET is_favorite = false WHERE user_id = 1234 and badge_id = 3457

To:

UPDATE user_badges SET is_favorite = false WHERE user_id = 1234 and badge_id = 3457 AND is_favorite = true

The query is the same expect is_favorite is added to the where with the opposite value of what we are updating it to, to prevent updating records that don’t need to be updated.

I do not understand how it would create a false assumption.

to prevent updating records that don’t need to be updated.

This is the false assumption :arrow_up:.

There couldn’t exist a record that doesn’t need to be updated :slightly_smiling_face: If everything works as expected, this query can’t return badges with different is_favorite values:

SELECT * 
FROM user_badges 
WHERE user_id = 1234 and badge_id = 3457

Either all of them have is_favorite == true, either all of them have is_favorite == false. Here is an example, user has 16 “God topic” badges: Screenshot 2021-07-15 at 23 39 40

It’s 16 rows in a database table. If user choose this badge as favorite all 16 rows will have is_favorite == true, if user remove this badge from favorites all 16 rows will have is_favorite == false. It’s not ideal implementation on the db level, we probably need a separate table. But this is what we have now.

As I understand, in this specific case adding an additional condition won’t speed up anything but can cause confusion. Or maybe there is something I don’t see?

In general, I understand that adding such condition even though looks counterintuitive may work faster in database.