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

Don't create duplicate Bundle authorization when non-available resources are moved to Bundle #988

Merged
merged 2 commits into from
May 12, 2022

Conversation

lalit97
Copy link
Contributor

@lalit97 lalit97 commented Apr 7, 2022

Description

When a partner is moved from an access method to Bundle we do some processing in update_existing_bundle_authorizations to update authorizations accordingly.

When the partner is set to one of the non-available statuses and is moved to Bundle, it creates duplicate Bundle authorizations for users. This shouldn't happen.

Rationale

When the partner is set to one of the non-available statuses and is moved to Bundle in the same operation,
it creates duplicate Bundle authorizations for the user. This should not happen ideally.

Phabricator Ticket

https://phabricator.wikimedia.org/T302882

How Has This Been Tested?

I have tested it by moving a partner from available to waitlist and setting its authorization_method to Bundle in the same query. Later checked that it does not have duplicate Authorizations in the DB.

Screenshots of your changes (if appropriate):

Types of changes

What types of changes does your code introduce? Add an x in all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Minor change (fix a typo, add a translation tag, add section to README, etc.)

@lalit97 lalit97 force-pushed the T302882 branch 2 times, most recently from dc83016 to 28b1c95 Compare April 7, 2022 18:29
@lalit97 lalit97 changed the title Don't create duplicate Bundle authorization when waitlisted resources are moved to Bundle Don't create duplicate Bundle authorization when non-available resources are moved to Bundle Apr 8, 2022
@suecarmol
Copy link
Contributor

Hi @lalit97! We are having some trouble trying to reproduce this error. Could you please add detailed instructions about how you reproduced this bug and the queries you made to check that

  1. the duplicate authorizations were created
  2. the bug was fixed when you made your changes.

@suecarmol
Copy link
Contributor

Hi @lalit97! We are having some trouble trying to reproduce this error. Could you please add detailed instructions about how you reproduced this bug and the queries you made to check that

1. the duplicate authorizations were created

2. the bug was fixed when you made your changes.

I was able to reproduce this bug and added the steps to reproduce it to the Phab task, although it will still be good to know how you reproduced this error in your own words :)

Copy link
Contributor

@suecarmol suecarmol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this tricky bug fix. I have some initial minor comments,

TWLight/users/signals.py Outdated Show resolved Hide resolved
TWLight/users/signals.py Outdated Show resolved Hide resolved
@lalit97
Copy link
Contributor Author

lalit97 commented May 1, 2022

Hi @lalit97! We are having some trouble trying to reproduce this error. Could you please add detailed instructions about how you reproduced this bug and the queries you made to check that

  1. the duplicate authorizations were created
  2. the bug was fixed when you made your changes.

Hi @suecarmol sure, I will add the steps here soon :)
updates: I have posted the 1st part. I will post the 2nd part this weekend.

@lalit97
Copy link
Contributor Author

lalit97 commented May 6, 2022

  1. the duplicate authorizations were created

First of all, I selected a Bundle eligible user (having wp_bundle_eligible=True). So the user was by default Authorized for all the partners who had the Bundle authorization_method.

In [1]: from TWLight.users.models import Authorization

In [2]: from django.contrib.auth.models import User

In [3]: user = User.objects.get(username="61329345")

In [4]: for auth in Authorization.objects.filter(user=user):
                 print(auth)
Out [4]:
authorized: Lalit97meta - authorizer: TWL Team - date_authorized: 2022-05-03 - company_name: Freudenberger, Geisler, Pohl, Sager KG, Salz, Zobel GbR

Then I created an application from that user for a non-Bundle partner (Austermühle) and approved it. So now there was a new Authorization for the user for that given partner.

In [5]: for auth in Authorization.objects.filter(user=user):
                 print(auth)
Out [5]: 
authorized: Lalit97meta - authorizer: TWL Team - date_authorized: 2022-05-03 - company_name: Freudenberger, Geisler, Pohl, Sager KG, Salz, Zobel GbR
authorized: Lalit97meta - authorizer: lalit_staff - date_authorized: 2022-05-03 - company_name: Austermühle

Then I changed the partner's status and authorization_method in a single operation like:

In [7]: from TWLight.resources.models import Partner
In [8]: partner = Partner.objects.get(id=48)

In [10]: partner.authorization_method == Partner.EMAIL
Out[10]: True

In [12]: partner.status == Partner.AVAILABLE
Out[12]: True

In [14]: partner.authorization_method = Partner.BUNDLE
In [15]: partner.status = Partner.WAITLIST
In [16]: partner.save()

I tried to print all the Authorizations for the user once again. I got these results:

In [17]: for auth in Authorization.objects.filter(user=user):
                   print(auth)
Out [17]:
authorized: Lalit97meta - authorizer: TWL Team - date_authorized: 2022-05-03 - company_name: Freudenberger, Geisler, Pohl, Sager KG, Salz, Zobel GbR
authorized: Lalit97meta - authorizer: lalit_staff - date_authorized: 2022-05-03 - company_name: Austermühle

The Austermühle partner is now Bundle. We know that only 1 Bundle type Authorization should be present for each user. But I still received 2 Authorization objects for the user. Both of them are for Bundle type partners. This is how I found that duplicate authorizations were created.

Copy link
Member

@jsnshrmn jsnshrmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long wait on getting this approved. This looks great! Thanks for your help!

@jsnshrmn jsnshrmn merged commit e2e55e7 into WikipediaLibrary:master May 12, 2022
@lalit97
Copy link
Contributor Author

lalit97 commented May 12, 2022

No issues at all. Thank you for your words 😀

@lalit97 lalit97 deleted the T302882 branch May 12, 2022 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants