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

Handle errors when adding user to group #346

Merged
merged 8 commits into from
Apr 26, 2021

Conversation

himdel
Copy link
Collaborator

@himdel himdel commented Apr 14, 2021

Fixes https://issues.redhat.com/browse/AAH-209

Added an addAlert method, used for all existing alerts, and added more request error handling to group-detail,
Also unsetting the selected value in the add modal on close,
and add a permission check for Add on the empty state screen.


109:    GroupAPI.get(this.state.params.id)
115:    GroupAPI.getPermissions(this.state.params.id)

if loading data fails on the intial load, replaced infinispinner with an empty screen with alerts..
badgroup


404:            UserAPI.list({ username__contains: name, page_size: 5 })
529:    UserAPI.list()

failure when autosuggesting groups
20210419200857


518:        return UserAPI.update(id.toString(), {
738:    UserAPI.list({

when saving from add modal
20210419195052

(modal now closes on failure, maybe it shouldn't?)


756:    UserAPI.update(user.id, user)

when removing user from group
20210419194945

(modal doesn't close on failure, maybe it should?)

@himdel himdel force-pushed the display-errors-209 branch from e3555e4 to 1ba8056 Compare April 19, 2021 19:43
@himdel
Copy link
Collaborator Author

himdel commented Apr 19, 2021

@ZitaNemeckova, @newswangerd can you review please?

Do we have any whole screen error screen that could be used when loading the group fails?
Have we dealt with the modals vs alerts issue? Should I just try to raise the alerts over the backdrop?

And we may want to consider making tasks to go through every file and find these API calls without catch if there's going to be more of us :)

@himdel
Copy link
Collaborator Author

himdel commented Apr 19, 2021

Also, I've been testing with network error (no webpack-dev-server), 504 (no api), 404 (no record), but not with any errors API could return, is there a convention? (As long as those end up in exception.message, they'll get displayed, but maybe that needs a bit lof logic to put it there?)

@himdel himdel changed the title [WIP] Handle errors when adding user to group Handle errors when adding user to group Apr 19, 2021
@ZitaNemeckova
Copy link
Member

ZitaNemeckova commented Apr 20, 2021

@himdel GET requests with 404 should redirect to NotFound page.

I would prefer to add loading component LoadingPAgeWithHeader more than just remove loading logic.

@ZitaNemeckova ZitaNemeckova added the backport-4.3 This PR should be backported to stable-4.3 (2.0) label Apr 22, 2021
@himdel himdel merged commit a01766f into ansible:master Apr 26, 2021
@himdel himdel deleted the display-errors-209 branch April 26, 2021 13:23
@patchback
Copy link

patchback bot commented Apr 26, 2021

Backport to stable-4.3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4.3/a01766f6c4c2cc6f86cb0df70bf542a6625f8282/pr-346

Backported as #373

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Apr 26, 2021
* Handle errors when adding user to group

Fixes https://issues.redhat.com/browse/AAH-209

* group-detail: drop unused `loading`

* group-detail: add addAlert, use instead of direct setState alerts:...

* group-detail: handle componentDidMount request errors, display instead of loading screen

errors during load pretty much mean we've finished loading

added e.message where possible

* group-detail: handle errors for remaining UserAPI requests

* addUserToGroup - don't close modal, return handled promise

so that renderModal can close itself

* Add Users modal - reset selected groups on close

prevents groups being reselected when opening again

* group-detail empty users: check permission before showing Add button

(cherry picked from commit a01766f)
@himdel
Copy link
Collaborator Author

himdel commented Apr 26, 2021

Merged for now, @ZitaNemeckova please ping me with the details of #346 (comment) once you're back from PTO :).

I see some specific 404 handling (edit namespace, delete user modal), but that doesn't redirect anywhere.
And not sure what you meant by the loading bit? (this is just removing an unused loading variable) :)

EDIT: 404 will be resolved as part of https://issues.redhat.com/browse/AAH-517, adding info there; loading is unrelated as this only removes a dead variable

newswangerd pushed a commit that referenced this pull request Apr 28, 2021
* Handle errors when adding user to group

Fixes https://issues.redhat.com/browse/AAH-209

* group-detail: drop unused `loading`

* group-detail: add addAlert, use instead of direct setState alerts:...

* group-detail: handle componentDidMount request errors, display instead of loading screen

errors during load pretty much mean we've finished loading

added e.message where possible

* group-detail: handle errors for remaining UserAPI requests

* addUserToGroup - don't close modal, return handled promise

so that renderModal can close itself

* Add Users modal - reset selected groups on close

prevents groups being reselected when opening again

* group-detail empty users: check permission before showing Add button

(cherry picked from commit a01766f)

Co-authored-by: Martin Hradil <mhradil@redhat.com>
@newswangerd newswangerd added the backported-4.3 This PR has been backported to stable-4.3 (2.0) label May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-4.3 This PR should be backported to stable-4.3 (2.0) backported-4.3 This PR has been backported to stable-4.3 (2.0)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants