feat: handle connected services in the admin panel #3329
Conversation
lorenzo-cavazzi
temporarily deployed
to
renku-ci-ui-3329
GitHub Actions
Inactive
|
You can access the deployment of this PR at https://renku-ci-ui-3329.dev.renku.ch |
lorenzo-cavazzi
temporarily deployed
to
renku-ci-ui-3329
GitHub Actions
Inactive
lorenzo-cavazzi
temporarily deployed
to
renku-ci-ui-3329
GitHub Actions
Inactive
* Finish the component to add a new service. * Polish the card to show existing services. * Add the delete button.
lorenzo-cavazzi
force-pushed
the
lorenzo/git-auth-flow-admin
branch
from
7e530cd to
13bfef7
Compare
lorenzo-cavazzi
temporarily deployed
to
renku-ci-ui-3329
GitHub Actions
Inactive
|
@leafty Thank you for the early review. I incorporated your suggestions and finished the PR. P.S. Sorry for rebasing; I changed the target branch to |
lorenzo-cavazzi
temporarily deployed
to
renku-ci-ui-3329
GitHub Actions
Inactive
There was a problem hiding this comment.
The update form does not work for the client secret. If the value is not changed, then do not send the field to the API. Also, do not populate the value with redacted.
Note that this is a blocker for the PR. We cannot have admins break connected services by just updating the display name for example.
* Add provider button text * Connected serivces page button color * italics text
lorenzo-cavazzi
temporarily deployed
to
renku-ci-ui-3329
GitHub Actions
Inactive
|
@leafty Thank you for the review. I applied your suggestions and fixed the problem with the client secret |
lorenzo-cavazzi
temporarily deployed
to
renku-ci-ui-3329
GitHub Actions
Inactive
| useEffect(() => { | ||
| if (!result.isSuccess) { | ||
| return; | ||
| } | ||
| toggle(); | ||
| }, [result.isSuccess, toggle]); |
There was a problem hiding this comment.
You should also reset the form here. Otherwise the client secret field is still populated when opening the form again.
There was a problem hiding this comment.
That shouldn't be necessary. After adding a new service, this invokes toggle() so that the other useEffect will trigger and invoke result.reset(). I tried a few combinations to test this.
I can still add a reset here to be safe if you think there are cases that I might be missing.
There was a problem hiding this comment.
Yes, add a form reset() in the useEffect() just below then 😄 . The result.reset() is for RTK Query. You also need to reset the form at the same time when you do a result.reset() in almost all cases.
There was a problem hiding this comment.
Oh sorry, I wrote result.reset() but I meant reset() from the other useEffect. When you open the new modal, the previous client_secret isn't there.
Anyway, adding a reset() here still makes the code more readable (and it makes sense to reset immediately after the successful action) so I added it in d640d83.
| control={control} | ||
| name="client_secret" | ||
| render={({ field }) => ( | ||
| <Input |
There was a problem hiding this comment.
Also, this field could be of type password with the button to toggle it to text like we have for some other hidden values. You can push that to a new issue if you like.
There was a problem hiding this comment.
That makes sense; I was undecided whether to use text or password here, but since you also mentioned this I'll switch to password -- no need for another PR 🙂
|
@leafty I implemented your second batch of suggestions; thanks for taking the time to go through the PR again. That helps in reducing the follow-up fixes/improvements :D I noticed that IDs with spaces are accepted, but editing/deleting those services triggers errors. Any idea what could be wrong?
|
This is a bug with the backend, I will have to fix it. |
|
Tearing down the temporary RenkuLab deplyoment for this PR. |
Handle connected services in the admin panel.
/deploy