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

[2064] Permissions Page init #2067

Merged
merged 34 commits into from
Mar 22, 2024
Merged

[2064] Permissions Page init #2067

merged 34 commits into from
Mar 22, 2024

Conversation

S78901
Copy link
Contributor

@S78901 S78901 commented Dec 6, 2023

In reference to:
#2064

@ZacharyKlein
Copy link
Collaborator

@S78901 Is this branch up to date with your most recent changes? I'm trying to level set on where the Swagger config is between develop and this PR.

@S78901
Copy link
Contributor Author

S78901 commented Mar 5, 2024 via email

@S78901 S78901 marked this pull request as ready for review March 14, 2024 05:06
Copy link
Member

@mkimberlin mkimberlin left a comment

Choose a reason for hiding this comment

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

The more that I have considered this, I don't think that this actually gains us much capability. I know that is going to be hard to hear, but because the roles are hardcoded, we don't have the ability to add a new role without altering this page. The page is tightly coupled to both the roles and the permissions as they currently exist. It results in the need to edit this page for any changes in either.

I really think this page should allow a user to select the role they are working with (options dynamically loaded from a service call), and then present them with a list of permissions with checkboxes (checked if they have the permission, unchecked if they don't). Those permissions would ideally be loaded from a service call as well.

We could merge this as is, but I think it's going to need to be adjusted right away, as this doesn't get to the main need: To be able to define new roles and assign them permissions.

@@ -96,6 +96,7 @@ const Root = styled('div')(({theme}) => ({

const adminLinks = [
// ["/admin/permissions", "Permissions"],
["/admin/edit-permissions", "Permissions Roles"],
Copy link
Member

Choose a reason for hiding this comment

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

Let's just call this menu item "Permissions".

@@ -101,6 +102,10 @@ export default function Routes() {
<Header title="Skills" />
<EditSkillsPage />
</Route>
<Route path="/admin/edit-permissions">
<Header title="Permissions Roles" />
Copy link
Member

Choose a reason for hiding this comment

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

Same here..."Permissions"

},
});
} else {
console.log(res?.error);
Copy link
Member

Choose a reason for hiding this comment

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

Leftover console.log(). I would remove this.

},
});
} else {
console.log(res?.error);
Copy link
Member

Choose a reason for hiding this comment

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

Ditto...Leftover console.log().

if (csrf) {
doTask1();
doTask2();
doTask3();
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 make these more descriptive names.

@mkimberlin mkimberlin merged commit be7726a into develop Mar 22, 2024
1 check passed
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.

None yet

3 participants