-
Notifications
You must be signed in to change notification settings - Fork 9
fix: user officer users and experiment table filter #1232
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
base: develop
Are you sure you want to change the base?
Changes from all commits
70abe16
8d86eee
e296a44
db25e46
0003da9
8ea968b
93e347b
6cb9c3d
4dfae27
38ab5a4
5e80d16
26170ac
ea251a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,14 @@ import { | |
| createUserObject, | ||
| } from './records'; | ||
|
|
||
| const fieldMap: { [key: string]: string } = { | ||
| created_at: 'created_at', | ||
| firstname: 'firstname', | ||
| preferredname: 'preferredname', | ||
| lastname: 'lastname', | ||
| institution: 'i.institution', | ||
| }; | ||
|
|
||
| export default class PostgresUserDataSource implements UserDataSource { | ||
| async delete(id: number): Promise<User | null> { | ||
| return database('users') | ||
|
|
@@ -494,26 +502,25 @@ export default class PostgresUserDataSource implements UserDataSource { | |
| } | ||
|
|
||
| async getUsers({ | ||
| filter, | ||
| searchText, | ||
| first, | ||
| offset, | ||
| userRole, | ||
| subtractUsers, | ||
| orderBy, | ||
| orderDirection = 'desc', | ||
| sortField = 'created_at', | ||
| sortDirection = 'desc', | ||
| }: UsersArgs): Promise<{ totalCount: number; users: BasicUserDetails[] }> { | ||
| return database | ||
| .select(['*', database.raw('count(*) OVER() AS full_count')]) | ||
| .from('users') | ||
| .join('institutions as i', { 'users.institution_id': 'i.institution_id' }) | ||
| .orderBy('users.user_id', orderDirection) | ||
| .modify((query) => { | ||
| if (filter) { | ||
| if (searchText) { | ||
| query.andWhere((qb) => { | ||
| qb.whereILikeEscaped('institution', '%?%', filter) | ||
| .orWhereILikeEscaped('firstname', '%?%', filter) | ||
| .orWhereILikeEscaped('preferredname', '%?%', filter) | ||
| .orWhereILikeEscaped('lastname', '%?%', filter); | ||
| qb.whereILikeEscaped('institution', '%?%', searchText) | ||
| .orWhereILikeEscaped('firstname', '%?%', searchText) | ||
| .orWhereILikeEscaped('preferredname', '%?%', searchText) | ||
| .orWhereILikeEscaped('lastname', '%?%', searchText); | ||
| }); | ||
| } | ||
| if (first) { | ||
|
|
@@ -530,8 +537,12 @@ export default class PostgresUserDataSource implements UserDataSource { | |
| if (subtractUsers && subtractUsers.length > 0) { | ||
| query.whereNotIn('users.user_id', subtractUsers); | ||
| } | ||
| if (orderBy) { | ||
| query.orderBy(orderBy, orderDirection); | ||
| if (sortField && sortDirection) { | ||
| if (!fieldMap.hasOwnProperty(sortField)) { | ||
| throw new GraphQLError(`Bad sort field given: ${sortField}`); | ||
| } | ||
| sortField = fieldMap[sortField]; | ||
| query.orderByRaw(`${sortField} ${sortDirection}`); | ||
| } | ||
| }) | ||
| .then( | ||
|
|
@@ -550,14 +561,24 @@ export default class PostgresUserDataSource implements UserDataSource { | |
|
|
||
| async getPreviousCollaborators( | ||
| userId: number, | ||
| filter?: string, | ||
| first?: number, | ||
| offset?: number, | ||
| sortField?: string, | ||
| sortDirection?: string, | ||
| searchText?: string, | ||
| userRole?: UserRole, | ||
| subtractUsers?: [number] | ||
| ): Promise<{ totalCount: number; users: BasicUserDetails[] }> { | ||
| if (userId == -1) { | ||
| return this.getUsers({ filter, first, offset, userRole, subtractUsers }); | ||
| return this.getUsers({ | ||
| searchText, | ||
| first, | ||
| offset, | ||
| userRole, | ||
| subtractUsers, | ||
| sortField, | ||
| sortDirection, | ||
| }); | ||
| } | ||
|
|
||
| const lastCollaborators = await this.getMostRecentCollaborators(userId); | ||
|
|
@@ -574,14 +595,26 @@ export default class PostgresUserDataSource implements UserDataSource { | |
| .join('institutions as i', { 'users.institution_id': 'i.institution_id' }) | ||
| .whereIn('users.user_id', userIds) | ||
| .modify((query) => { | ||
| if (filter) { | ||
| if (searchText) { | ||
| query.andWhere((qb) => { | ||
| qb.whereILikeEscaped('institution', '%?%', filter) | ||
| .orWhereILikeEscaped('firstname', '%?%', filter) | ||
| .orWhereILikeEscaped('preferredname', '%?%', filter) | ||
| .orWhereILikeEscaped('lastname', '%?%', filter); | ||
| qb.whereILikeEscaped('institution', '%?%', searchText) | ||
| .orWhereILikeEscaped('firstname', '%?%', searchText) | ||
| .orWhereILikeEscaped('preferredname', '%?%', searchText) | ||
| .orWhereILikeEscaped('lastname', '%?%', searchText); | ||
| }); | ||
| } | ||
| logger.logInfo( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is not important then I would not log the sort information. |
||
| `Sort field: ${sortField}, direction: ${sortDirection}`, | ||
| {} | ||
| ); | ||
| if (sortField && sortDirection) { | ||
| if (!fieldMap.hasOwnProperty(sortField)) { | ||
| throw new GraphQLError(`Bad sort field given: ${sortField}`); | ||
| } | ||
| sortField = fieldMap[sortField]; | ||
| query.orderByRaw(`${sortField} ${sortDirection}`); | ||
| } | ||
|
|
||
| if (first) { | ||
| query.limit(first); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,11 +40,14 @@ export class UsersArgs { | |
| @Field(() => [Int], { nullable: 'itemsAndList' }) | ||
| subtractUsers?: [number]; | ||
|
|
||
| @Field(() => String, { nullable: true }) | ||
| orderBy?: string; | ||
| @Field({ nullable: true }) | ||
| public sortField?: string; | ||
|
|
||
| @Field(() => String, { nullable: true }) | ||
| orderDirection?: string; | ||
| @Field({ nullable: true }) | ||
| public sortDirection?: string; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, we could use SortDirection enum (asc, desc) for validating the input, I guess. |
||
|
|
||
| @Field({ nullable: true }) | ||
| public searchText?: string; | ||
| } | ||
|
|
||
| @ArgsType() | ||
|
|
@@ -65,20 +68,24 @@ export class UsersQuery { | |
| @Args() | ||
| { | ||
| userId, | ||
| filter, | ||
| first, | ||
| offset, | ||
| userRole, | ||
| subtractUsers, | ||
| sortField, | ||
| sortDirection, | ||
| searchText, | ||
| }: PreviousCollaboratorsArgs, | ||
| @Ctx() context: ResolverContext | ||
| ) { | ||
| return context.queries.user.getPreviousCollaborators( | ||
| context.user, | ||
| userId, | ||
| filter, | ||
| first, | ||
| offset, | ||
| sortField, | ||
| sortDirection, | ||
| searchText, | ||
| userRole, | ||
| subtractUsers | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import { useSearchParams } from 'react-router-dom'; | |
|
|
||
| import { Experiment, SettingsId } from 'generated/sdk'; | ||
| import { useFormattedDateTime } from 'hooks/admin/useFormattedDateTime'; | ||
| import { setSortDirectionOnSortField } from 'utils/helperFunctions'; | ||
| import useDataApiWithFeedback from 'utils/useDataApiWithFeedback'; | ||
|
|
||
| import ExperimentReviewContent, { | ||
|
|
@@ -58,7 +59,7 @@ export default function ExperimentsTable({ | |
| const page = searchParams.get('page'); | ||
| const pageSize = searchParams.get('pageSize'); | ||
| const selectedExperimentId = searchParams.get('experiment'); | ||
|
|
||
| const [isFirstRender, setIsFirstRender] = useState(true); | ||
| const refreshTableData = () => { | ||
| tableRef.current?.onQueryChange({}); | ||
| }; | ||
|
|
@@ -75,10 +76,14 @@ export default function ExperimentsTable({ | |
| React.useEffect(() => { | ||
| let isMounted = true; | ||
|
|
||
| if (isMounted) { | ||
| if (isMounted && !isFirstRender) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you use the same code what you usied in the other PR: |
||
| refreshTableData(); | ||
| } | ||
|
|
||
| if (isFirstRender) { | ||
| setIsFirstRender(false); | ||
| } | ||
|
|
||
| return () => { | ||
| isMounted = false; | ||
| }; | ||
|
|
@@ -193,6 +198,12 @@ export default function ExperimentsTable({ | |
| ]; | ||
| } | ||
|
|
||
| columns = setSortDirectionOnSortField( | ||
| columns, | ||
| searchParams.get('sortField'), | ||
| searchParams.get('sortDirection') | ||
| ); | ||
|
|
||
| const experimentReviewTabs = [ | ||
| EXPERIMENT_MODAL_TAB_NAMES.EXPERIMENT_INFORMATION, | ||
| EXPERIMENT_MODAL_TAB_NAMES.PROPOSAL_INFORMATION, | ||
|
|
@@ -215,12 +226,21 @@ export default function ExperimentsTable({ | |
| options={{ | ||
| searchText: search || undefined, | ||
| pageSize: pageSize ? +pageSize : 10, | ||
| initialPage: search ? 0 : page ? +page : 0, | ||
| initialPage: page ? +page : 0, | ||
| }} | ||
| onRowsPerPageChange={(pageSize) => { | ||
| setSearchParams((searchParams) => { | ||
| searchParams.set('pageSize', pageSize.toString()); | ||
| searchParams.set('page', '0'); | ||
|
|
||
| return searchParams; | ||
| }); | ||
| }} | ||
| onSearchChange={(searchText) => { | ||
| setSearchParams((searchParams) => { | ||
| if (searchText) { | ||
| searchParams.set('search', searchText); | ||
| searchParams.set('page', '0'); | ||
| } else { | ||
| searchParams.delete('search'); | ||
| } | ||
|
|
@@ -235,6 +255,25 @@ export default function ExperimentsTable({ | |
| return searchParams; | ||
| }); | ||
| }} | ||
| onOrderCollectionChange={(orderByCollection) => { | ||
| const [orderBy] = orderByCollection; | ||
|
|
||
| if (!orderBy) { | ||
| setSearchParams((searchParams) => { | ||
| searchParams.delete('sortField'); | ||
| searchParams.delete('sortDirection'); | ||
|
|
||
| return searchParams; | ||
| }); | ||
| } else { | ||
| setSearchParams((searchParams) => { | ||
| searchParams.set('sortField', orderBy.orderByField); | ||
| searchParams.set('sortDirection', orderBy.orderDirection); | ||
|
|
||
| return searchParams; | ||
| }); | ||
| } | ||
| }} | ||
| /> | ||
|
|
||
| {selectedExperiment && ( | ||
|
|
||
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.
Is it possible to validate the user’s input values with GraphQL in this case?
The sortDirection string goes directly into the SQL command which could be harmful.