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

Set ghost to false on club approval decision #745

Closed
wants to merge 1 commit into from

Conversation

aviupadhyayula
Copy link
Member

@aviupadhyayula aviupadhyayula commented Nov 6, 2024

Currently, approval decisions patch the approved and approved_by fields, but the ghost field remains unchanged. This causes the "changes to this club are pending approval" modal to show up for clubs that have been approved.

@aviupadhyayula aviupadhyayula added the bug Something isn't working label Nov 6, 2024
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.88%. Comparing base (3a002c4) to head (c1d6289).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #745   +/-   ##
=======================================
  Coverage   71.88%   71.88%           
=======================================
  Files          32       32           
  Lines        6933     6933           
=======================================
  Hits         4984     4984           
  Misses       1949     1949           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@julianweng
Copy link
Member

It looks like we do have logic to set ghost to be false within ClubSerializer, here. The edge case which prompted this PR was likely a result of an admin approving a club manually from Django Admin, which does not use the save function within the serializer.

This is unrelated but we may have a separate bug / edge case at L1557 where we use the nullity of approved to signify revoking approval, but we use approved=false elsewhere including on the frontend, which might have led to other issues seen previously: curious to know your thoughts on it?

@aviupadhyayula
Copy link
Member Author

aviupadhyayula commented Nov 6, 2024

Good catch, looks like the serializer makes this PR unnecessary. Regarding the approved field: I don't think that's correct? IIRC approved=None signifies clubs that are seeking reapproval, while approved=False is reserved for clubs that have had their most recent approval request denied.

@aviupadhyayula
Copy link
Member Author

Going to close this, and take a look at how we use approved on the frontend in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants