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

Add new query getOrganizationsByEmailPaginated #172

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

## [Unreleased]

### Added

- Add new `getOrganizationsByEmailPaginated` query to allow pagination on organizations by email query

## [0.55.0] - 2024-08-22

### Added
Expand Down
9 changes: 9 additions & 0 deletions graphql/schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ type Query {
getOrganizationsByEmail(email: String): [B2BOrganization]
@checkUserAccess
@cacheControl(scope: PRIVATE)

getOrganizationsByEmailPaginated(email: String, page: Int, pageSize: Int): B2BOrganizationPagination
@checkUserAccess
@cacheControl(scope: PRIVATE)

checkOrganizationIsActive(id: String): Boolean
@cacheControl(scope: PRIVATE)
Expand Down Expand Up @@ -475,6 +479,11 @@ type B2BOrganization {
costCenterName: String
}

type B2BOrganizationPagination {
data: [B2BOrganization]
pagination: Pagination
}

type SimpleRole {
id: ID
name: String!
Expand Down
17 changes: 17 additions & 0 deletions node/clients/storefrontPermissions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import updateUser from '../mutations/updateUser'
import getB2BUser from '../queries/getB2BUser'
import getOrganizationsByEmail from '../queries/getOrganizationsByEmail'
import getOrganizationsByEmailPaginated from '../queries/getOrganizationsByEmailPaginated'
import getPermission from '../queries/getPermission'
import getRole from '../queries/getRole'
import getUser from '../queries/getUser'
Expand All @@ -22,7 +23,7 @@
super('[email protected]', ctx, options)
}

public checkUserPermission = async (app?: string): Promise<any> => {

Check warning on line 26 in node/clients/storefrontPermissions.ts

View workflow job for this annotation

GitHub Actions / QE / Lint Node.js

Unexpected any. Specify a different type
return this.query({
extensions: this.getPersistedQuery(app),
query: getPermission,
Expand All @@ -30,7 +31,7 @@
})
}

public getOrganizationsByEmail = async (email: string): Promise<any> => {

Check warning on line 34 in node/clients/storefrontPermissions.ts

View workflow job for this annotation

GitHub Actions / QE / Lint Node.js

Unexpected any. Specify a different type
return this.query({
extensions: this.getPersistedQuery(),
query: getOrganizationsByEmail,
Expand All @@ -40,7 +41,23 @@
})
}

public getOrganizationsByEmailPaginated = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

email: string,
page: number,
pageSize: number
): Promise<any> => {

Check warning on line 48 in node/clients/storefrontPermissions.ts

View workflow job for this annotation

GitHub Actions / QE / Lint Node.js

Unexpected any. Specify a different type
return this.query({
extensions: this.getPersistedQuery(),
query: getOrganizationsByEmailPaginated,
variables: {
email,
page,
pageSize,
},
})
}

public listRoles = async (): Promise<any> => {

Check warning on line 60 in node/clients/storefrontPermissions.ts

View workflow job for this annotation

GitHub Actions / QE / Lint Node.js

Unexpected any. Specify a different type
return this.query({
extensions: this.getPersistedQuery(),
query: listRoles,
Expand All @@ -48,7 +65,7 @@
})
}

public getRole = async (roleId: string): Promise<any> => {

Check warning on line 68 in node/clients/storefrontPermissions.ts

View workflow job for this annotation

GitHub Actions / QE / Lint Node.js

Unexpected any. Specify a different type
return this.query({
extensions: this.getPersistedQuery(),
query: getRole,
Expand All @@ -56,7 +73,7 @@
})
}

public listAllUsers = async (): Promise<any> => {

Check warning on line 76 in node/clients/storefrontPermissions.ts

View workflow job for this annotation

GitHub Actions / QE / Lint Node.js

Unexpected any. Specify a different type
return this.query({
extensions: this.getPersistedQuery(),
query: listAllUsers,
Expand All @@ -72,7 +89,7 @@
roleId?: string
organizationId?: string
costCenterId?: string
}): Promise<any> => {

Check warning on line 92 in node/clients/storefrontPermissions.ts

View workflow job for this annotation

GitHub Actions / QE / Lint Node.js

Unexpected any. Specify a different type
return this.query({
extensions: this.getPersistedQuery(),
query: listUsers,
Expand Down Expand Up @@ -102,7 +119,7 @@
pageSize?: number
sortOrder?: string
sortedBy?: string
}): Promise<any> => {

Check warning on line 122 in node/clients/storefrontPermissions.ts

View workflow job for this annotation

GitHub Actions / QE / Lint Node.js

Unexpected any. Specify a different type
return this.query({
extensions: this.getPersistedQuery(),
query: listUsersPaginated,
Expand Down
25 changes: 25 additions & 0 deletions node/queries/getOrganizationsByEmailPaginated.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { print } from 'graphql'
import gql from 'graphql-tag'

export default print(gql`
query organizationsPaginated($email: String!, $page: Int, $pageSize: Int) {
getOrganizationsByEmailPaginated(
email: $email
page: $page
pageSize: $pageSize
) {
data {
costId
orgId
roleId
id
clId
}
pagination {
page
pageSize
total
}
}
}
`)
105 changes: 105 additions & 0 deletions node/resolvers/Queries/Organizations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,111 @@ const Organizations = {
}
},

getOrganizationsByEmailPaginated: async (
_: void,
{
email,
page = 1,
pageSize = 25,
}: {
email: string
page: number
pageSize: number
},
{
clients: { storefrontPermissions, session },
vtex: { logger, sessionToken, adminUserAuthToken },
}: any
) => {
const organizationFilters: string[] = []
let fromSession = false
const {
data: { checkUserPermission },
}: any = await storefrontPermissions
.checkUserPermission('[email protected]')
.catch((error: any) => {
logger.error({
error,
message: 'checkUserPermission-error',
})

return {
data: {
checkUserPermission: null,
},
}
})

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

!(email?.length > 0)
) {
const sessionData = await session
.getSession(sessionToken as string, ['*'])
.then((currentSession: any) => {
return currentSession.sessionData
})
.catch((error: any) => {
logger.warn({
error,
message: 'getOrganizationsByEmail-session-error',
})

return null
})

if (checkUserPermission?.permissions.includes('add-users-organization')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const orgId =
sessionData?.namespaces?.['storefront-permissions']?.organization
?.value

if (!orgId) {
throw new Error('No permission for getting the organizations')
}

organizationFilters.push(orgId)
Comment on lines +299 to +307
Copy link
Contributor

@enzomerca enzomerca Sep 4, 2024

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

}

if (!(email?.length > 0)) {
email = sessionData?.namespaces?.profile?.email?.value
fromSession = true
}
}
Comment on lines +310 to +314
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


const response =
await storefrontPermissions.getOrganizationsByEmailPaginated(
email,
page,
pageSize
)

const organizations =
response?.data?.getOrganizationsByEmailPaginated?.data?.filter(
({ orgId }: { orgId: string }) => {
return (
fromSession ||
(organizationFilters.length > 0
? organizationFilters.find((id: string) => orgId === id)
: true)
)
}
)

try {
return {
data: organizations,
pagination: response.data?.getOrganizationsByEmailPaginated?.pagination,
}
} catch (error) {
logger.error({
error,
message: 'getOrganizationsByEmail-error',
})
throw new GraphQLError(getErrorMessage(error))
}
},

getOrganizationByIdStorefront: async (
_: void,
{ id }: { id: string },
Expand Down
Loading