update account-detail endpoint to show info on Account if the Org has an Account #730
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #730 +/- ##
===========================================
Coverage 96.03000 96.03000
===========================================
Files 814 814
Lines 18424 18449 +25
===========================================
+ Hits 17693 17718 +25
Misses 731 731
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| current_org = self.context["view"].owner | ||
| if current_org.account: | ||
| raise serializers.ValidationError( | ||
| detail="You cannot update your plan manually, for help or changes to plan, connect with sales@codecov.io" | ||
| ) |
There was a problem hiding this comment.
Hmm... I might be misunderstanding how Django Serializers work, but I'm a bit surprised that this error didn't get raised in the test_retrieve_org_with_account test. AccountPlanSerializer references a plan that is serialized by PlanSerializer. If there is an account on the org (which there is), then I would guess that when the plan attempts to serialize, then it would fail here... 🤔
There was a problem hiding this comment.
heheh, this is definitely a django serializers thing. there are different flows to serialize incoming and outgoing objects. the object only goes through validate when you try to update it, so when the object is incoming to the database with changes. It doesn't go through here on the way out to be displayed.
There was a problem hiding this comment.
Oooff, this is definitely not intuitive.
Would we also want to pull some values in this "plan" from the accounts object instead? i.e. plan type, billing rate?
There was a problem hiding this comment.
heheheh, it is :)
There was a problem hiding this comment.
so for Orgs, we need to think about the GET experience and the PATCH experience, (no POSTs to this endpoint)
the GET experience (what we return to the User to see), they see the pretty_plan from the Account if the Org has an Account, if they don't have an account they see the pretty_plan on the Org.
The PATCH experience (user wants to update plan):
- without Account: can update the plan on the Org according to all the layered rules and validation we have already 😅
- with Account: firstly, they shouldn't be able to see options to update the plan since they are sales led, so this should never happen, but just in case they manage to send this in, reject changes with this error message.
There was a problem hiding this comment.
this logic is twisty-turny because things like the pretty_plan replacement are happening in shared and plan updating comes through gazebo (+1 for monorepo!!), lmk if you wanna do a call to trace the pathways 👍 👍 👍
What does this PR do?
There are certain fields that are controlled by the Account if the Org has an Account. These changes make it so that when you view the
planormemberpage, if your Org has an Account, you see the details from the Account, not the Org.Also adds logic to make sure that a user cannot do self-serve functions (changing their plan) if they have an Account. The current implementation of Accounts are that they are sales-led only, they have to change their plan details through their sales contact, and if they were able to change the plan on a single Org in the Account, weird things would happen. So if they have an Account we want to block these actions.