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

[TASK-1096] Implementation of queries for organization #5197

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

pauloamorimbr
Copy link
Contributor

@pauloamorimbr pauloamorimbr commented Oct 24, 2024

Checklist

  1. If you've added code that should be tested, add tests
  2. If you've changed APIs, update (or create!) the documentation
  3. Ensure the tests pass
  4. Run ./python-format.sh to make sure that your code lints and that you've followed our coding style
  5. Write a title and, if necessary, a description of your work suitable for publishing in our release notes
  6. Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
  7. Open an issue in the docs if there are UI/UX changes
  8. Create a testing plan for the reviewer and add it to the Testing section

Description

This PR implements reactQuery hooks to be used on organization development.

Notes

The useOrganizationQuery hook was modified:

  • A separated function was created to process organization data before returning it
  • If the mmosEnabled feature flag is is true we created a mocked response, so we have better control on frontend development. This mock can be updated by the developer to create proper testing situations
  • A enabled property was added to the query, to make sure it can only run after the sessionStore is loaded, since we need the currentAccount.organization.url to load the organization details in the new implementation, and it should not affect the current implementation.

Testing

Check for the mock data:

useOrganizationQuery is currently being used in accountSidebar, so one way of testing this implementation is to use:

  const orgQuery = useOrganizationQuery();
  console.log(orgQuery.data);

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 in session.tsx to prevent the sessionStorage.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.

@pauloamorimbr pauloamorimbr self-assigned this Oct 24, 2024
Copy link

Copy link
Contributor

@Akuukis Akuukis left a comment

Choose a reason for hiding this comment

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

jsapp/js/query/queries/meQuery.ts Outdated Show resolved Hide resolved
@Akuukis Akuukis self-requested a review October 25, 2024 10:57
@pauloamorimbr pauloamorimbr marked this pull request as draft October 25, 2024 14:25
@pauloamorimbr pauloamorimbr marked this pull request as ready for review October 28, 2024 19:45
Copy link
Member

@duvld duvld left a comment

Choose a reason for hiding this comment

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

Just some questions

jsapp/js/account/stripe.api.ts Outdated Show resolved Hide resolved
jsapp/js/account/stripe.api.ts Outdated Show resolved Hide resolved
jsapp/js/account/stripe.api.ts Outdated Show resolved Hide resolved
jsapp/js/account/stripe.api.ts Outdated Show resolved Hide resolved
@@ -147,6 +147,8 @@ export interface Organization {
modified: string;
slug: string;
is_owner: boolean;
is_mmo: boolean;
Copy link
Member

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

Copy link
Contributor Author

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.

@duvld
Copy link
Member

duvld commented Oct 28, 2024

Look good to me now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants