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: 🍰 Switch User Role As Admin #4136

Merged
merged 17 commits into from
Feb 10, 2021
Merged

feat: 🍰 Switch User Role As Admin #4136

merged 17 commits into from
Feb 10, 2021

Conversation

Mogge
Copy link
Contributor

@Mogge Mogge commented Jan 15, 2021

🍰 Pullrequest

An admin can change the role a user has

Issues

Todo

  • Do we have to ensure that there is at least one admin remaining? What happens if the only admin changes their role to user?

@Mogge
Copy link
Contributor Author

Mogge commented Jan 15, 2021

Can an admin change her own role? This way we could avoid easily that at least one admin is remaining.

@ulfgebhardt ulfgebhardt changed the base branch from master to 234-fix January 18, 2021 17:02
@ulfgebhardt ulfgebhardt changed the base branch from 234-fix to master January 18, 2021 17:02
Copy link
Contributor Author

@Mogge Mogge left a comment

Choose a reason for hiding this comment

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

Looks like a good start! Please see my comments.
Can you add tests for the role change to user.spec.js.
I have to add tests in the backend.


export const updateUserRole = (role, id) => {
return gql`
mutation {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better this way:

        mutation($role: UserGroup!, $id: ID!) {
            switchUserRole(role: $role, id: $id) {

So we have the schema correctly in the frontend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please have two spaces as indentation

Copy link
Member

Choose a reason for hiding this comment

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

If you could solve this by following @Mogge suggestion … @tippanamadhusudan

webapp/pages/admin/users.vue Show resolved Hide resolved
webapp/pages/admin/users.vue Show resolved Hide resolved
@@ -48,6 +48,23 @@
<template #createdAt="scope">
{{ scope.row.createdAt | dateTime }}
</template>

<template slot="role" slot-scope="scope">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! I really like the way you are using slots here!

<ApolloQuery :query="FetchAllRoles">
<template v-slot="{ result: { data } }">
<template v-if="data">
<select
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you try ds-select to get the standard layout for select fields?

Copy link
Member

Choose a reason for hiding this comment

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

… and try this as well @tippanamadhusudan

@Mogge
Copy link
Contributor Author

Mogge commented Jan 29, 2021

@tippanamadhusudan I added the tests to the backend. I also made sure, that the current user cannot change the own role to make sure that there is always at least one admin left. May be you can catch this in the frontend as well. The backend throws an error when current user tries to change the own role.

@Tirokk Tirokk changed the title feat [WIP]: Switch User Role As Admin feat: [WIP] 🍰 Switch User Role As Admin Feb 8, 2021
@Tirokk Tirokk marked this pull request as draft February 8, 2021 08:54
Copy link
Member

@Tirokk Tirokk left a comment

Choose a reason for hiding this comment

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

Hey @tippanamadhusudan ,
your first PR is close to get merged into the master !!! 😍

And it’s a really important one, because we need it urgent for the first network wir.social going online at the beginning of next week! 🚀🚀💫

If you could follow @Mogge suggestions 👇🏼 you have it quickly ready. 🙏🏼💪🏼

Thank you very much for helping !!! 🍀


export const updateUserRole = (role, id) => {
return gql`
mutation {
Copy link
Member

Choose a reason for hiding this comment

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

If you could solve this by following @Mogge suggestion … @tippanamadhusudan

<ApolloQuery :query="FetchAllRoles">
<template v-slot="{ result: { data } }">
<template v-if="data">
<select
Copy link
Member

Choose a reason for hiding this comment

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

… and try this as well @tippanamadhusudan

webapp/pages/admin/users.vue Show resolved Hide resolved
webapp/pages/admin/users.vue Show resolved Hide resolved
@Tirokk Tirokk marked this pull request as ready for review February 8, 2021 09:46
@Mogge Mogge changed the title feat: [WIP] 🍰 Switch User Role As Admin feat: 🍰 Switch User Role As Admin Feb 9, 2021
Copy link
Member

@Tirokk Tirokk left a comment

Choose a reason for hiding this comment

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

You both got it together !!!

A big thank to @tippanamadhusudan !!! This will go into our first live version of ocelot.social: wir.social next week.
Congratulations !!! 🚀🚀💫💫
… and to @Mogge as well for sure! 😘

Just a little hint … 👇🏼

Gorzilla

},
}),
)
// await expect(mocks.$toast.success).toHaveBeenCalled()
Copy link
Member

Choose a reason for hiding this comment

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

May delete that comment in favour of the next test?

Comment on lines +181 to +183
update({ __type }) {
return __type.enumValues.map((item) => item.name)
},
Copy link
Member

Choose a reason for hiding this comment

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

That seems to be cool stuff! 😎🥰

@ulfgebhardt ulfgebhardt force-pushed the 4124-switch-user-role branch from 8b7570d to dd8f32b Compare February 10, 2021 12:11
@Mogge Mogge merged commit 35626e6 into master Feb 10, 2021
@Mogge Mogge deleted the 4124-switch-user-role branch February 10, 2021 12:27
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.

🚀 [Feature] Admin Can Give Roles To Users
4 participants