-
Notifications
You must be signed in to change notification settings - Fork 1
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
✨(frontend) add mail domain access management #413
Conversation
6b8db24
to
aee446c
Compare
aee446c
to
631e63c
Compare
3e631d3
to
bb945d5
Compare
@@ -5,7 +5,7 @@ import { useTranslation } from 'react-i18next'; | |||
import styled from 'styled-components'; | |||
|
|||
import { Box } from '@/components'; | |||
import { MailDomainsLayout } from '@/features/mail-domains'; | |||
import { MailDomainsLayout } from '@/features/mail-domains/index'; |
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.
2b10d31
to
6ece017
Compare
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.
src/frontend/apps/desk/src/features/mail-domains/access-management/__tests__/accesses.test.tsx
Outdated
Show resolved
Hide resolved
secrets
Outdated
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.
secrets
should not be versionned.
src/frontend/apps/desk/src/features/mail-domains/access-management/components/AccessAction.tsx
Outdated
Show resolved
Hide resolved
setRole, | ||
}: ChooseRoleProps) => { | ||
const { t } = useTranslation(); | ||
const rolesToDisplay = Array.from(new Set([currentRole, ...availableRoles])); |
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.
const rolesToDisplay = Array.from(new Set([currentRole, ...availableRoles])); | |
const rolesToDisplay = [currentRole, ...availableRoles]; |
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.
As for this one, the currently connected user may actually have a role in the available roles.
For example, I am an owner (currentRole = OWNER), and with this privilege, I can assign a user the VIEWER, ADMIN, or OWNER role.
I'm not too fond of this set and would like to find a better design for these props. I probably do not need to use the currentRole prop but to show the default role of the currently connected user.
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.
It is necessary to create a default option for the current role. Otherwise, we can not show the currently connected user his role. I have to keep the set to ensure we have no double for the moment.
<Button | ||
aria-label={t('Open the modal to delete this access')} | ||
onClick={() => { | ||
setIsModalDeleteOpen(true); | ||
setIsDropOpen(false); | ||
}} | ||
color="primary-text" | ||
icon={ | ||
<span className="material-icons" aria-hidden="true"> | ||
delete | ||
</span> | ||
} | ||
> | ||
<Text $theme="primary">{t('Remove from domain')}</Text> | ||
</Button> |
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.
If you don't have the right to delete, this button shouldn't be available.
I can see you are lacking some informations from the backend actually, for the team feature by example, we have the object abilities
, that provide us lot of informations about what the current user can do or not, thanks to that we display the actions buttons or not:
{
"abilities": {
"delete": true,
"get": true,
"patch": true,
"put": true,
"set_role_to": [
"owner",
"member"
]
}
}
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.
The mail domain possesses an ability field. I added the conditions to hide/show the buttons. I just forgot.
I could only test these changes with component tests. Currently, it is impossible to create mailboxes due to the app's configuration with DiMail or to create new accesses, as the POST endpoint does not exist yet.
Sabrina is aware of the impossibility of deleting or updating accesses. I just did what I could to prepare the front end for these soon-to-be-released endpoints. Shortly, components to create accesses will probably have to be added to this view once the POST endpoint is developed on the backend. |
d1ef239
to
fc23a31
Compare
fc23a31
to
513488c
Compare
d29c3ae
to
86a726c
Compare
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.
It could have been nice to have the abilities
per accesses, to know if we can do an action or not from a frontend perspective. By example, if I am the last owner, I cannot remove myself, but I don't know it before I consume the endpoint.
86a726c
to
a14866e
Compare
- separate feature into domain, mailboxes as a member management feature is arriving in the mail domains feature - pages/mail-domains/[slug].tsx becomes pages/mail-domains/[slug]/index.tsx
- access management view is ready to use get, patch and delete requests once backend is ready. How to create accesses with post will come later in a future commit. - update translations and component tests. - reduce gap between mail domains feature logo and mail domain name in top banner
a14866e
to
0582a3a
Compare
Purpose
A user with the appropriate privileges should be able to administrate the user's access to his mail domain, like changing a member's access role (viewer, administrator, owner) or deleting his access.
Proposal
Add a view /mail-domains//accesses showing a list of accesses like the group members list on /teams, along with its features.
Note
Adding this new feature, which is very similar to members' management in a group, makes me think that soon, the mail domain mailboxes and access management will need to be merged into the group management feature.
Also, at the moment, only the get accesses backend endpoint is ready to be consumed.
Before
before.mail.domain.accesses.change.mp4
After
after.mail.domains.accesses.change.mp4