-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
[TASK-1096] Implementation of queries for organization #5197
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- As funny as they are, I'm ok with naming because they are consistent and descriptive.
- blocked by refactor(account): query-ify OrganizationContext TASK-1194 #5164
- blocked by decision in internal chat
142d191
to
8f0b4a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some questions
@@ -147,6 +147,8 @@ export interface Organization { | |||
modified: string; | |||
slug: string; | |||
is_owner: boolean; | |||
is_mmo: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be optional until the backend will actually return it? Or just returning false in the query is safe enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, and from my tests, backend is already returning these fields. I'm setting it to false in the current query because when talking to @jamesrkiger we thought it would be a good idea to force the default "false" values until we have the front end properly implemented to avoid issues.
Look good to me now :) |
@pauloamorimbr I'm testing this branch out locally (with Stripe enabled, though I don't think that's pertinent) and I am seeing a redundant organization query when I navigate from the projects page (home) to account settings without the mmos feature enabled. Doesn't seem to happen on main. I suspect it's because the profile page calls |
@pauloamorimbr It looks like the work for adding orgs to the /me endpoint got merged yesterday. I'm inclined to say we should implement the real fetch now, at least behind the feature flag (though maybe we don't even need the feature flag?). What do you think? |
762a30e
to
cb617c4
Compare
Checklist
./python-format.sh
to make sure that your code lints and that you've followed our coding styleDescription
This PR implements reactQuery hooks to be used on organization development.
Notes
The
useOrganizationQuery
hook was modified:enabled
property was added to the query, to make sure it can only run after thesessionStore
is loaded, since we need thecurrentAccount.organization.url
to load the organization details in the new implementation.is_mmo
false when the feature flag is off while in development to ensure we're not displaying any in development functionalitysessionStore.currentAccount.organization.url
and then we use that URL to fetch the organization data.Testing
Check for the mock data:
useOrganizationQuery
is currently being used inaccountSidebar
, so one way of testing this implementation is to use:This way one can see the result of the data mocking with and without the
mmosEnabled
feature flag.Check for the conditional query
On can comment out or add a
setTimeout
insession.tsx
to prevent thesessionStorage.isPending
from being set to false, causing the organization query to not run, waiting for it.Other than code-related tests, there should be no changes in the current functionality.