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

Add reset password button to ops project list #4910

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

dgannon991
Copy link
Contributor

What this PR does / why we need it:
Adds a reset password button to the project list in the ops view

Which issue(s) this PR fixes:

Fixes #4844

Does this PR introduce a user-facing change?:
Yes, a new button and column in the ops list

  • How are users affected by this change: the OPs UI is updated
  • Is this breaking change: No
  • How to migrate (if breaking change): N/A

Just pushing up a draft for this at the moment to get some early feedback. I'll have a look through to see how/if we're testing the ops side of things later, but wanted to provide an early view of the UI.

image

Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
@dgannon991
Copy link
Contributor Author

@khanhtc1202 & @ffjlabo - forgot that it wouldn't notify you in draft mode :D Nothing urgent, just hoped to get some feedback on the UI before writing up the tests. Cheers!

@dgannon991 dgannon991 marked this pull request as ready for review May 12, 2024 18:20
@ffjlabo
Copy link
Member

ffjlabo commented May 13, 2024

@dgannon991
Thank you so much for the quick response!! The button UI is LGTM for me :)

Thanks to your PR, I noticed this action is extremely dangerous because the control plane operator might reset for other projects accidentally. 👀
It would be nice to prevent users from accidentally initializing static admins for other projects.

So, how about adding a confirmation screen?
e.g. the project list page -> reset password confirmation page -> success page

Reset password confirmation page like this↓ (the sentences are the draft, and I want more correct them 🙏 )

Reset Static Admin for the project XXX.
You must check the project name before you click the button.

"Reset button"

@khanhtc1202
Copy link
Member

@dgannon991 @ffjlabo
How about asking the user to reinstate the project name as part of the reset password confirmation page? (Cloud service like AWS use that method as re-check step if I remember correctly)

@dgannon991
Copy link
Contributor Author

I like the sound of that. I've got some down time over the next couple of days, so I'll give it a go and let you know :)

@ffjlabo
Copy link
Member

ffjlabo commented May 14, 2024

@dgannon991 @khanhtc1202

How about asking the user to reinstate the project name as part of the reset password confirmation page? (Cloud service like AWS use that method as re-check step if I remember correctly)

Thank you both 🙏 I agree with it!

Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
@dgannon991
Copy link
Contributor Author

Hi Guys,
I've added a confirmation page, and you have to retype the project ID.
One note though, the user experience is not great when you get things wrong (due to the server just returning error responses instead of markup.) Are we happy with this, as the ops pages are for technical users?
Cheers!

@ffjlabo
Copy link
Member

ffjlabo commented May 17, 2024

@dgannon991
Thank you for the nice commitment :)
I also agree the ops pages are plain because these are for technical users.

[IMO] If possible, it would be nice to show the Reset page with the failed message, or the link to the Reset page.

Project Page
Cursor_と_localhost_9082_projects

Fail
Cursor_と_localhost_9082_projects_reset-password_ID_test

Succsess
localhost_9082_projects_reset-password_ID_test

@dgannon991
Copy link
Contributor Author

Absolutely. I'll give that a whirl over the weekend. Cheers for the continued feedback on this one guys :)

Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
@dgannon991
Copy link
Contributor Author

Morning! I've updated this to show a nice error message on the confirmation page. Do let me know if it's too big and red! I looked at the material UI library we use in the normal frontend for inspiration, but it is the first think like it on the ops pages!

@ffjlabo
Copy link
Member

ffjlabo commented May 25, 2024

Hi @dgannon991
Sorry for the late reply. Currently doing the other task.
So plz wait a moment 🙏

@dgannon991
Copy link
Contributor Author

Hi @ffjlabo,

Not a problem at all, the other task looks mega! No rush from my side at all :)

D

Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
Copy link

codecov bot commented May 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 90 lines in your changes are missing coverage. Please review.

Project coverage is 29.22%. Comparing base (9bf5c33) to head (9c8d51f).

Current head 9c8d51f differs from pull request most recent head d6fd2f7

Please upload reports for the commit d6fd2f7 to get more accurate results.

Files Patch % Lines
pkg/app/ops/handler/handler.go 0.00% 90 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4910      +/-   ##
==========================================
- Coverage   29.38%   29.22%   -0.17%     
==========================================
  Files         322      321       -1     
  Lines       40852    40917      +65     
==========================================
- Hits        12006    11958      -48     
- Misses      27885    28000     +115     
+ Partials      961      959       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
@dgannon991
Copy link
Contributor Author

Hi Guys,
Just working on the coverage now. I've brought it up, but I think it will now be easy to really get it much higher, so I'll have another go tomorrow night at getting it there :)
Cheers!
David

Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
@dgannon991
Copy link
Contributor Author

Hi Guys,
Managed to get a few more tests written, and I've got pretty much everything I added covered. It does use gomock (which is a shame) but I couldn't think of a better way to test without actually creating a project store!
Let me know what you think!
D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check or reset the ID/PW of a project once registered when forgetting them
4 participants