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

UI: Fix new bugs from disable/enable feature #537

Merged
merged 4 commits into from
Sep 16, 2020
Merged

Conversation

daniel-arnauer
Copy link
Contributor

@daniel-arnauer daniel-arnauer commented Aug 13, 2020

  • It is not possible to add the permissions global.enableUser and global.disableUser in UI
  • Closed resources must be ignored when checking assignments (API level bug)
  • After canceling the window to add global permissions, the userlist re-fetches, this is not necessary
  • The CSS Layout in "Add User" window is broken
  • The user permissions of disabled user should be possible to edit
  • When disabling/enabling an user, a confirmation dialog should appear
  • Instead of showing the assignments in the snack bar, the assignment should be listed within the confirmation container

Closes #536

@openkfwCI
Copy link

openkfwCI commented Aug 13, 2020

NotesTime
Note for Reviewer: E2E tests on remote server succeededWed, 16 Sep 2020 10:01:30 +0000

Generated by E2E-Test

@daniel-arnauer
Copy link
Contributor Author

daniel-arnauer commented Aug 21, 2020

Remaining:

  • Api unit test
  • Translations
  • Rebase

@daniel-arnauer daniel-arnauer force-pushed the global_perm_fix branch 2 times, most recently from 9db3e7d to 6b8fd60 Compare August 31, 2020 14:08
@@ -86,7 +86,7 @@ export function addHttpHandler(server: FastifyInstance, urlPrefix: string, servi
const userPermissionsResult = await service.getUserPermissions(ctx, user, userId);

if (Result.isErr(userPermissionsResult)) {
throw new VError(userPermissionsResult, "user.intent.revokePermission failed");
throw new VError(userPermissionsResult, "user.intent.listPermission failed");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correction of previous refactor PR

Copy link
Contributor

@Stezido Stezido left a comment

Choose a reason for hiding this comment

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

Is an own endpoint for listAssigments needed?
Suggestion: Open up the confirmation dialog when the disableUser saga is called. After confirmation the frontend tries to disable the user, but get an error which leads to a filled assignments table in the confirmation dialog.
When the new endpoint should be kept the permission for the intent listAssignments should be the intent global.listAssignments not global.disableUser. If the issuer has the global.listAssignments permission but cannot view the projects, subproject or workflowitem the api cannot give information about where the user is assigned to.
Use the existing ConfirmationDialog.

userId: UserDisable.RequestData,
issuer: ServiceUser,
issuerOrganization: string,
user: UserAssignmentsGet.RequestData,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
user: UserAssignmentsGet.RequestData,
requestData: UserAssignmentsGet.RequestData,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -13,7 +13,11 @@ const styles = {
paperRoot: {
width: "100%",
overflow: "scrollable"
}
},
container: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty styles

@@ -0,0 +1,241 @@
import { Typography } from "@material-ui/core";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the ConfirmationDialog component

@@ -0,0 +1,79 @@
import { Typography } from "@material-ui/core";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the ConfirmationDialog component

@@ -18,7 +18,11 @@ const styles = {
paperRoot: {
width: "100%",
overflow: "scrollable"
}
},
container: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty styles

infoIcon: {
fontSize: 20,
marginRight: "10px"
},
info: {
display: "flex",
paddingRight: 20
}
},
customWidth: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty styles

@@ -70,6 +70,15 @@ export const ENABLE_USER_SUCCESS = "ENABLE_USER_SUCCESS";
export const DISABLE_USER = " DISABLE_USER";
export const DISABLE_USER_SUCCESS = " DISABLE_USER_SUCCESS";

export const SHOW_ENABLE_DIALOG = "SHOW_ENABLE_DIALOG";
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed if using the existing confirmation dialog

Copy link
Contributor

@Stezido Stezido left a comment

Choose a reason for hiding this comment

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

There is a console warning in frontend when creating a new group.
when canceling the createUser/createGroup dialog listpermissions are fetched and the site is rerendered. If the dialog is canceled nothing should be fetched

);
});

it("A user can view user assignments of worklfowitems with view permissions", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

workflowitems

for await (const subproject of subprojects) {
if (subproject.status === "closed") continue;
if (subproject.assignee === userId) {
assignedSubprojects.push(subproject);
if (!isRoot && !Subproject.permits(subproject, issuer, subprojectIntents)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

cannot be root because a root user cannot be assigned to a subproject

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, this checks if the issuer is the root-user

for await (const workflowitem of workflowitems) {
if (workflowitem.status === "closed") continue;
if (workflowitem.assignee === userId) {
assignedWorkflowitems.push(workflowitem);
if (!isRoot && !Workflowitem.permits(workflowitem, issuer, workflowitemIntents)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

cannot be root because a root user cannot be assigned to a workflowitem

for await (const project of projects) {
if (project.status === "closed") continue;
if (project.assignee === userId) {
assignedProjects.push(project);
if (!isRoot && !Project.permits(project, issuer, projectIntents)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

cannot be root because a root user cannot be assigned to a project

expect(xhr.response.body.error.code).to.eql(412);
});
cy.get(`[data-test=confirmation-dialog-confirm]`).should("be.disabled");
cy.wait("@fetchAssignments");
Copy link
Contributor

Choose a reason for hiding this comment

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

why wait for that call? It's the end of the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change the position

expect(xhr.response.body.error.code).to.eql(412);
});
cy.get(`[data-test=confirmation-dialog-confirm]`).should("be.disabled");
cy.wait("@fetchAssignments");
Copy link
Contributor

Choose a reason for hiding this comment

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

why wait for that call? It's the end of the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change the position

@@ -34,6 +34,9 @@ import { applyOriginalActions, createAdditionalActions } from "./createAdditiona
import { executeOriginalActions } from "./executeOriginalActions";

class ConfirmationContainer extends React.Component {
componentDidMount() {
this.props.cancelConfirmation(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to reset the redux state of Confirmation Dialog component. Bugs occurred after pressing the submit button and the API rejects the request - in this case, the submit-state remains true in redux store, hence the dialog is not showing up anymore (and the request are send immediately - without confirmation)

Solution: cancelConfirmation() only sets defaultState. For better readability, I should create another function called resetConfirmation() that also set the redux state to defaultState.
Another solution: Add action CONFIRMATION_UNSUCESSFULL and yield it on every API error related to confirmation dialog. This action resets redux state of confirmation dialog like this state.set("confirmed", defaultState.get("confirmed"));

<a href={url} target="_blank" rel="noopener noreferrer">
{assignment.displayName}
</a>
<br />
Copy link
Contributor

Choose a reason for hiding this comment

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

use css styles instead of br tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Stezido Stezido assigned daniel-arnauer and unassigned Stezido Sep 15, 2020
@Stezido Stezido assigned daniel-arnauer and unassigned Stezido Sep 15, 2020
daniel-arnauer added 4 commits September 16, 2020 11:25
When a user is fetching assignments, he only gets data from projects,
subprojects and workflowitems when he has view permissions, respectively.
Refactored the Dialogs for enabling/disabling users.
@daniel-arnauer daniel-arnauer merged commit 9b87d28 into master Sep 16, 2020
@Stezido Stezido deleted the global_perm_fix branch October 7, 2020 09:40
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.

UI: new bugs
3 participants