Page MenuHomePhabricator

Don't create a duplicate Bundle authorization when waitlisted resources are moved to Bundle
Closed, ResolvedPublic

Description

When a partner is moved from an access method to Bundle we do some processing in https://github.com/WikipediaLibrary/TWLight/blob/fe6369f1365ad8f53b630a009583ed3ba4811d7b/TWLight/users/signals.py#L72 to update authorizations accordingly.

When the partner is set to the waitlist status and is moved to Bundle, it creates duplicate Bundle authorizations for users. This shouldn't happen.

Steps to reproduce this bug

  1. Create and approve two applications for different partners
  2. Change both partners' status to Waitlistedor Not Available and authorization_method to Bundle
  3. Change only one partner's status to Available
  4. Log out of your account and then login again
  5. A repeated bundle authorization appears and the partner is repeated in the My Library UI. The query below returns duplicates for the partner you just changed to Available:
SELECT users_authorization.user_id, users_authorization_partners.partner_id, COUNT(users_authorization_partners.partner_id) AS auth_count FROM users_authorization_partners INNER JOIN resources_partner ON users_authorization_partners.partner_id = resources_partner.id INNER JOIN users_authorization ON users_authorization_partners.authorization_id = users_authorization.id WHERE resources_partner.authorization_method = 3 GROUP BY users_authorization.user_id, users_authorization_partners.partner_id HAVING auth_count > 1;

Event Timeline

Hi! @Samwalton9 can I work on this ticket

When the partner is set to the waitlist status and is moved to Bundle

Is the partner's status is changed to Waitlist from any other status or does it remains waitlisted during the transition

Sure @lalit97 :)

Is the partner's status is changed to Waitlist from any other status or does it remains waitlisted during the transition

This bug relates to changing both authorization method and availability in the same save action. If we switch from Waitlisted to Available, and then switch to Library Bundle, the bug doesn't occur.

This bug relates to changing both authorization method and availability in the same save action. If we switch from Waitlisted to Available, and then switch to Library Bundle, the bug doesn't occur.

Thanks for the clarification. I looked into it and tried to understand what the signal is doing.

quoting from Description

it creates duplicate Bundle authorizations for users.

you mean it creates 2 Authorization objects for the same User with the same Partner?

Ah yes sorry for not explaining what I meant by a 'Bundle authorization'

Authorizations are almost always uniquely between one user and one partner, and only one such User-Partner authorization can exist. There's a special case for 'Library Bundle' authorizations, where the user has an authorization to all the default partners. This is an authorization from one user to multiple partners. If you set some partners to the Bundle authorization method locally and log out and back in you should see this pop up in the list of Authorization objects. There should still only ever be one of these authorizations for each user.

So yes, in this case what's happening is that a user is getting an extra Bundle authorization when we make this change, instead of just having one.

No issues :)

Great! that made the picture clear to me, thank you for the explanation. I really had no idea that Bundle works this way till now :D

Okay, I am looking into https://github.com/WikipediaLibrary/TWLight/blob/master/TWLight/applications/signals.py#L123 this one and the users app signal you linked in the description mainly. Also found out that bundle authorizations for a user are getting updated on each login via https://github.com/WikipediaLibrary/TWLight/blob/master/TWLight/users/models.py#L600 will check this also. I will try to add all the pieces and find out the problem.

Hi, @Samwalton9 I have raised a pull request for this ticket at https://github.com/WikipediaLibrary/TWLight/pull/988. Do these changes require a new test case?

The bug only occurs when we move one Available Partner to a non-available (waitlist or not available) status and change its authorization_method to Bundle in the same operation.

When we perform both above-written operations together, both the variables present in the signal add_to_auths and remove_from_auths will result in False. So the Authorization objects related to this Partner will remain unchanged. That will create more than 1 Bundle Authorization object for the same user (say 2 in this case).

Later if there is another Partner moved to the Bundle (which is not causing this bug). It will get added to both of the Bundle Authorization objects of the user.

Another critical issue raised due to this bug is that given Partner's previous authorizations will not get deleted. But the partner is now Bundle, due to this, some editors who are not Bundle eligible (having wp_bundle_eligible=False) will get access to the Bundle.

For fixing this bug, since the Partner is now Bundle, first of all, we should delete the previous Authorizations related to it. Also, we should not add this Partner in the present Bundle Authorizations objects since it is in a non-available status right now.

Later if the Partner's status is set to available. It will get added to the corresponding Bundle Authorization objects automatically with help of the signal.

One suggestion is once this PR is deployed, you should check for any existing buggy records in the production. I cannot think of any direct way to fix existing records. Triggering save() on existing partners won't help.

If we set the waitlisted Partner (which is causing the duplication of Authorization) to available. The signal will merge the authorizations. But we can not always do that since what if it's not available yet.

Thanks for the explanation!

Do these changes require a new test case?

Yes I think it would be great if this has a test case.

my pleasure :) okay, I will update the PR with a test soon.

Hi @Samwalton9 As per our discussion I have added a test case for this behavior in the Pull request.