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

feat(admin): add backend role management api #4487

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

chinook25
Copy link
Contributor

What are the main changes you did:

  • Added role management for updating users as admin

how to test:

  • unit test should succeed
  • can also test using the gql playground with query mutation updateUser($updateUser:InputUpdateUser) {updateUser(updateUser:$updateUser){status, message}} and an updated user as variable (see unit test for field you can update).

todo:

  • updated docs in case of new feature
  • added/updated tests
  • added/updated testplan to include a test for this fix, including ref to bug using # notation

Copy link
Member

@mswertz mswertz left a comment

Choose a reason for hiding this comment

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

Mostly small comments.
Can you check coverage gaps?

image image

(rest of coverage gaps seem exceptions which are less risky not to cover)

.field(
GraphQLFieldDefinition.newFieldDefinition()
.name(USERS)
.argument(GraphQLArgument.newArgument().name(EMAIL).type(Scalars.GraphQLString))
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a 'search' argument so we can search in any field. I would for example like to search on role names. That might also be a future enhancement (I think we should first convert emx2 metadata schema to be a 'normal' schema).

assertEquals(TEST_PERSOON, anotherSchemaMember.getUser());

// clean up
testDatabase.removeUser(TEST_PERSOON);
Copy link
Member

Choose a reason for hiding this comment

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

I would keep the schema, generally we don't cleanup schemas because it helps debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it, but shouldn't unit test run in a vacuum? I kind of dislike that running the tests clogs up my database with schemas.

@chinook25 chinook25 requested a review from mswertz November 18, 2024 09:30
@chinook25 chinook25 merged commit 640553f into master Nov 18, 2024
5 of 6 checks passed
@chinook25 chinook25 deleted the feat/admin-role-management-backend branch November 18, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants