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

[FIX-141/161] Budgeteer administration menu. #298

Closed
wants to merge 74 commits into from
Closed

Conversation

maximAtanasov
Copy link
Member

@maximAtanasov maximAtanasov commented Sep 18, 2018

This PR should be ready for review now.

The administration menu is started as a separate web app (this one is set to run on port 8090) and
can be started from the root folder by running ./gradlew budgeteer-admin-interface:bootRun.
The first user to log in to it is made global admin.
The global admin currently has the following abilities:

  • Set the password/email of any user in the Budgeteer
  • Delete any user in the budgeteer
  • Make any other user admin as well
  • Delete any project
  • Change the permissions of users in a project
  • Remove/Add users from/to a project
  • Edit the project name and start/end dates.

This adresses issue #141 and issue #161

@maximAtanasov maximAtanasov changed the title [FIX-141] WIP: Budgeteer administration menu. [FIX-141/161] WIP: Budgeteer administration menu. Nov 13, 2018
@maximAtanasov maximAtanasov changed the title [FIX-141/161] WIP: Budgeteer administration menu. [FIX-141/161] Budgeteer administration menu. Nov 13, 2018
@acetous
Copy link
Member

acetous commented Jan 18, 2019

  • Benutzeradministration gebaut

Copy link
Contributor

@LeonieAdis LeonieAdis left a comment

Choose a reason for hiding this comment

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

Bugs
User administration

  • E-Mail-Entry: the entry wants a string like "a@b.cd". If you don't enter the @-char and at least one char after it, a error message appears. If you enter "a@b" or "a@b.c" the input is not accepted and reset, but there is no error message or explanation, why this input isn't correct

Project administration in the administration application

  • Clicking the breadcrumb "Project administration" leads to a NullPointerException.
  • I would write "Project title", not "Project-title"

Project administration in the normal application

  • I removed the admin status of the current logged in user. After accepting the warning, I was still able to access the project administration menu and change my status again. I set my status to an admin again and the button to the administration menu disappeared.
    After logging out and logging in again, I could reach the menu again and in the view of another admin, the first user was an admin, too.
  • I would write "Project title", not "Project-title"

All pages

  • the project title is called "Project-title" in the project administration, but "Project name" in the table on the bottom of the administration menu
  • The input length of the entries isn't restricted, which leads to exceptions if you enter to long strings.

Usability

  • I don't like the dropdowns for changing the role of a user in a project. One can either be a user or an admin and a user. I would suggest changing the dropdown to a checkbox, so one can either be set to an admin, as @tinne mentioned in Missing option to administer users #141 before. I even could break the application by unsetting both the admin and the user status.
  • After changing the role of a user in a project, I would prefer seeing a message, that the new role has been saved successfully. There is a little bit too less feedback for the user. Initially, I don't know if I have to click a save button or if my changes have been applied automatically.

Styling/GUI:

  • User administration menu: There are strange lines under the entry-button-groups.
  • User administration menu: There are no labels for the entries and there is not much place for labels, so maybe placeholders in the entries might be helpful to know what to enter where
  • User administration menu: the buttons don't have the same height as the entries, what looks unbalanced.
  • Project administration menu: The "Save"- and the "Add user"- buttons have sharp corners and are completely flat, while all other buttons have rounded edges and a 3D effect on the bottom border.
  • Project administration menu: The role-dropdown is higher than the button next to it

Additional comments
I tested this PR by running both the "normal" application and the admin application.
It wasn't possible to be logged in both applications at the same time. When I logged into the second application, I was redirected in the first application to the log in page after clicking something.
I tried using different accounts or the same account for the two applications.
I don't know, if that is intended or must be changed at all, I just wanted to document and discuss this "feature".

@acetous
Copy link
Member

acetous commented Feb 5, 2019

Hey @LeonieAdis, can you please adapt the suggested changes?

adis added 14 commits February 8, 2019 11:28
# Conflicts:
#	budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/web/pages/administration/ProjectAdministrationPage.java
#	budgeteer-web-interface/src/main/java/org/wickedsource/budgeteer/web/pages/administration/ProjectAdministrationPage.properties
#	budgeteer-web-interface/src/test/java/org/wickedsource/budgeteer/web/pages/administration/ProjectAdministrationPageTest.java
@LeonieAdis
Copy link
Contributor

I implemented all my suggestions, except for the missing feedback messages for the e-mail-entries.
I know, a similar e-mail-entry with correct feedback messages is used in the "Register"-page, but I wasn't able to transfer that functionality.
I tried different ways, but couldn't figure out how to get it working.
Maybe someone else can help here.
In my opinion, the missing feedback messages aren't a big issue, so it may also just be kept like it is right now.

The earlier version of this PR was - due to addition of not-nullable attributes - not compatible with an already filled database. I fixed this by setting those attributes nullable.

Additional there was a logic mistake in the project administration:
If the current logged in user was a project admin, his admin rights could only be unset, if there was another project admin. But if the current logged in user wasn't a project admin and there was another project admin, the admin rights of this second admin could be removed, so there was no project admin left.
I changed this logic:
Now the application checks, if there is more than one project admin. Then all project admins can become non-admins.
If there is only one project admin left, you can't remove his admin rights.
In this way, there always has to be an project admin.
The only exception is a project, that was created in a earlier version of the application, when we didn't have user roles. Then by default, there is no project admin.
In this case, you first have to give admin rights to a user in the project.

This was linked to issues Sep 7, 2020
@czarnecki czarnecki closed this Dec 9, 2020
@czarnecki czarnecki deleted the fix-141 branch December 9, 2020 12:25
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.

Roles for users Missing option to administer users
7 participants