search

SwissDataScienceCenter / renku-ui

The web frontend of the Renku platform
https://renkulab.io
Apache License 2.0
13 stars 6 forks source link

UI changes for auth #3118

Closed ciyer closed 3 months ago

ciyer commented 4 months ago

/deploy renku=release-0.52.x

RenkuBot commented 3 months ago

You can access the deployment of this PR at https://renku-ci-ui-3118.dev.renku.ch

ciyer commented 3 months ago

I'm reviewing the code right now. As an early comment, I believe the "Keywords" section got accidentally duplicated while merging/rebasing the code. I'll follow up soon with the full review

Great catch! Just fixed this.

ciyer commented 3 months ago

While testing, I noticed the visibility is listed twice on the General Settings tab, and with different formats. Perhaps something mixed up while rebasing?

Yes, looks like this happened in the rebase. I have fixed this.

ciyer commented 3 months ago
  • Users can now demote themselves from the Owner role to something with less permissions. It would be nice to prevent demoting the last owner; otherwise, projects would un-deletable.
  • I suggest making users always able to remove themselves from a project they don't own. Otherwise, they could have a role assigned by someone else and be stuck with that project (sounds bad for spamming unwanted content). Here is an example: I can't remove my user from there: https://renku-ci-ui-3118.dev.renku.ch/v2/projects/lorenzo.cavazzi.tech/test-creation

I agree that these are both problems, but I think they need to be fixed in the backend first. The UI should allow any change that is allowed by the backend and should generally prevent the user from making changes that are not allowed by the backend as a UX convenience.

  • (cosmetic) The content spacing is a little off; the outer box is in line with the other components, but the internal content adds further spacing. I suggest removing that and adding just the bottom margin to the different sections (or top margins, or using the gap logic; whatever solution makes the most sense here). I assume this was evident only after merging the other Project 2.0 changes at the last moment 😁 Not a big deal tbh

Yes, this is a good point. I will try to make those cosmetic fixes quickly.

lorenzo-cavazzi commented 3 months ago

I agree that these are both problems, but I think they need to be fixed in the backend first. The UI should allow any change that is allowed by the backend and should generally prevent the user from making changes that are not allowed by the backend as a UX convenience.

I agree the backend should catch up by disallowing unwanted actions, but I don't think this excuses us from giving users a better UX. Ofc the backend should support the actions (E.G. no point in adding a UX to let users remove themselves from a project if the backend errors), but I don't see a negative point in preventing a downgrade of the last Owner even if doing it from the backend API works.

leafty commented 3 months ago

I agree that these are both problems, but I think they need to be fixed in the backend first. The UI should allow any change that is allowed by the backend and should generally prevent the user from making changes that are not allowed by the backend as a UX convenience.

I agree the backend should catch up by disallowing unwanted actions, but I don't think this excuses us from giving users a better UX. Ofc the backend should support the actions (E.G. no point in adding a UX to let users remove themselves from a project if the backend errors), but I don't see a negative point in preventing a downgrade of the last Owner even if doing it from the backend API works.

I think you are missing the point: we should not hide the bug in the UI because doing so may hide the existence of the bug from the backend maintainers. The backend needs to have the bug filed before we hide the bug.

ciyer commented 3 months ago

Great feature! Is it possible to display the member names in the project information? Currently, it only shows the number of members, not their names.

Sure, I will make a PR to address this.

RenkuBot commented 3 months ago

Tearing down the temporary RenkuLab deplyoment for this PR.

lorenzo-cavazzi commented 3 months ago

I think you are missing the point: we should not hide the bug in the UI because doing so may hide the existence of the bug from the backend maintainers. The backend needs to have the bug filed before we hide the bug.

I totally get that point :smile: and it still sounds poor to me. I don't understand how implementing a poor UX can be the best solution here, especially when the reasoning behind is that the backend allows for it. In this case, it also allows for a better solution -- we don't need any change for that. I think creating an issue in the backend and communicating with the developers is a much better solution to get the bug addressed than letting users fall for it in the UI.

leafty commented 3 months ago

I have filed an issue at last https://github.com/SwissDataScienceCenter/renku-data-services/issues/227