Add new query getOrganizationsByEmailPaginated by vhaalmeida · Pull Request #172 · vtex-apps/b2b-organizations-graphql

vhaalmeida · 2024-08-29T12:32:35Z

What problem is this solving?

This new query will allow to retrieve the users' organizations paginated.
Currently when an user is associated to a big number of organizations is generating a timeout.

This PR should be merged only when this other one is merged: vtex-apps/storefront-permissions#157

How to test it?

Access https://timeoutsfp--jamalstore.myvtex.com/admin/graphql-ide
Run the query below in the b2b-organizations-graphql app

{
  getOrganizationsByEmailPaginated(
    email: "[email protected]",
    page: 1
    pageSize: 25
  ) {
    data {
      id
      clId
     	costId
      orgId
      organizationName
      costCenterName
    }
    pagination {
      page
      pageSize
      total
    }
  }
}

Workspace

Screenshots or example usage:

Describe alternatives you've considered, if any.

Related to / Depends on

vtex-apps/storefront-permissions#157

How does this PR make you feel? 🔗

…ons-by-email-paginated

vtex-io-ci-cd · 2024-08-29T12:32:38Z

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

Patch (backwards-compatible bug fixes)
Minor (backwards-compatible functionality)
Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

No thanks, I would rather do it manually 😞

vtex-io-docs-bot · 2024-08-29T12:32:38Z

Beep boop 🤖

I noticed you didn't make any changes at the docs/ folder

There's nothing new to document 🤔
I'll do it later 😞

In order to keep track, I'll create an issue if you decide now is not a good time

I just updated 🎉🎉

github-actions · 2024-08-29T12:33:07Z

	Messages
📖	❤️ Thanks!
📖	🎉 PR additions = `160`, PR deletions = `0`

Generated by 🚫 dangerJS against 4818d99

wender

Please run the Lint in the files changed to auto-fix the line-break issues?
If vtex setup don't install all dependencies, you need to run yarn in the root and node folder

sonarqubecloud · 2024-08-30T17:32:14Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
40.5% Duplication on New Code

See analysis details on SonarCloud

wender

LGTM

enzomerca

A general comment: I would try to avoid using session data as much as possible in these new APIs as this would make it harder to use it in a headless scenario and in integrations where the system has no session token to provide on the call. (we can discuss offline if you have any concerns by removing the session token usage/verification)

enzomerca · 2024-09-04T14:19:16Z

    })
  }

+  public getOrganizationsByEmailPaginated = async (


Do we need this function here and on the storefront-permissions app? (PR)

Can't we query Master Data directly from this app and avoid additional call to another app?

@enzomerca I'm following the same approach of the getOrganizationsByEmail query

Yes, but we are removing the cyclic dependencies, and this is one of them, and thinking about the domain and context, the storefront shouldn't know about the organization.

enzomerca · 2024-09-04T14:22:10Z

+
+    if (
+      (!adminUserAuthToken &&
+        !checkUserPermission?.permissions.includes('add-sales-users-all')) ||


why are we checking if the user has add-sales-users-all permission if this is just a getOrganizationsByEmail query?

Hi @enzomerca
Is the same approach of the getOrganizationsByEmail query. If I'm not mistaken this is for the case of a query run through the webstore where there's no admin auth cookie.

enzomerca · 2024-09-04T14:22:59Z

+          return null
+        })
+
+      if (checkUserPermission?.permissions.includes('add-users-organization')) {


why are we checking if the user has add-users-organization permission if this is just a getOrganizationsByEmail query?

Hi @enzomerca
Is the same approach of the getOrganizationsByEmail query. If I'm not mistaken this is for the case of a query run through the webstore where there's no admin auth cookie.

Why don't you remove it to test? It appears to be a bug or old mistake.

enzomerca · 2024-09-04T14:30:36Z

+        const orgId =
+          sessionData?.namespaces?.['storefront-permissions']?.organization
+            ?.value
+
+        if (!orgId) {
+          throw new Error('No permission for getting the organizations')
+        }
+
+        organizationFilters.push(orgId)


session will have just one org id, the current logged org, right? and below we are filtering the result to return only the orgs added to organizationsFilters, so the getOrganizationsByEmail would not return all the organizations for the given email in this case, right? (which would not make much sense as this is called getOrganizationsByEmail)

Hi @enzomerca
Is the same approach of the getOrganizationsByEmail query. This is the case for the request from the storefront where there's no admin auth cookie.

@vhaalmeida We would like to avoid old mistakes, and we have a lot of tasks to improve the source code and the B2B suite app implementations. So our advice is to adjust the rules to test.

enzomerca · 2024-09-04T14:33:20Z

+      if (!(email?.length > 0)) {
+        email = sessionData?.namespaces?.profile?.email?.value
+        fromSession = true
+      }
+    }


the function is named getOrganizationsByEmail but we are allowing the user to not provide an email and get the email from the session. Who ever is going to call this API can do that on their side, right?

I prefer to remove this, make the email a required field and simplify the code.

Hi @enzomerca
Is the same approach of the getOrganizationsByEmail query.

Rudge · 2024-10-16T18:09:27Z

    })
  }

+  public getOrganizationsByEmailPaginated = async (


Yes, but we are removing the cyclic dependencies, and this is one of them, and thinking about the domain and context, the storefront shouldn't know about the organization.

Rudge · 2024-10-16T18:14:15Z

+          return null
+        })
+
+      if (checkUserPermission?.permissions.includes('add-users-organization')) {


Why don't you remove it to test? It appears to be a bug or old mistake.

Rudge · 2024-10-16T18:17:14Z

+        const orgId =
+          sessionData?.namespaces?.['storefront-permissions']?.organization
+            ?.value
+
+        if (!orgId) {
+          throw new Error('No permission for getting the organizations')
+        }
+
+        organizationFilters.push(orgId)


@vhaalmeida We would like to avoid old mistakes, and we have a lot of tasks to improve the source code and the B2B suite app implementations. So our advice is to adjust the rules to test.

vhaalmeida added 3 commits August 29, 2024 09:19

Add new query getOrganizationsByEmailPaginated

facb552

Merge remote-tracking branch 'origin/master' into feat/get-organizati…

bac2cf1

…ons-by-email-paginated

Changelog

c90de63

revert test timeout

f0db88c

wender requested changes Aug 30, 2024

View reviewed changes

wender self-requested a review August 30, 2024 17:12

lint

4818d99

wender approved these changes Aug 30, 2024

View reviewed changes

enzomerca requested changes Sep 4, 2024

View reviewed changes

Rudge suggested changes Oct 16, 2024

View reviewed changes

Uh oh!

Conversation

vhaalmeida commented Aug 29, 2024

What problem is this solving?

How to test it?

Screenshots or example usage:

Describe alternatives you've considered, if any.

Related to / Depends on

How does this PR make you feel? 🔗

Uh oh!

vtex-io-ci-cd Bot commented Aug 29, 2024 • edited by wender Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vtex-io-docs-bot Bot commented Aug 29, 2024

Uh oh!

github-actions Bot commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wender left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Aug 30, 2024

Quality Gate passed

Uh oh!

wender left a comment

Choose a reason for hiding this comment

Uh oh!

enzomerca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enzomerca Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vtex-io-ci-cd Bot commented Aug 29, 2024 •

edited by wender

Loading

github-actions Bot commented Aug 29, 2024 •

edited

Loading

enzomerca Sep 4, 2024 •

edited

Loading