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

error out when trying to delete a location that has children #437

Merged
merged 4 commits into from
Aug 27, 2019

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Aug 23, 2019

No description provided.

@sean797
Copy link
Member

sean797 commented Aug 23, 2019

Can we solve this by propagating error messages returned from the server instead?

@evgeni
Copy link
Member Author

evgeni commented Aug 23, 2019

Yes and no. In this particular case the http code is 200 :(

@evgeni
Copy link
Member Author

evgeni commented Aug 23, 2019

@mdellweg
Copy link
Member

Can we do a scoped search to find children? Also do we want to give an option to delete the children, too?

@evgeni
Copy link
Member Author

evgeni commented Aug 24, 2019

You can't search for parent id, no :(
Recursive delete might be possible, but would require more code and I'm lazy (and the UI doesn't support that either).

What we could do instead of the search is to issue the delete and inspect the API answer, as it does contain the error, just not with the right http code.

@evgeni
Copy link
Member Author

evgeni commented Aug 26, 2019

I've pushed two iterations moving the error detection to parsing the result instead of predicting the problem and surfacing the error message received from the API. Still a workaround, but feels nicer?

changed, entity = self.resource_action(resource, 'destroy', payload)

# this is a workaround for https://projects.theforeman.org/issues/26937
if entity and 'error' in entity and 'message' in entity['error']:
Copy link
Member

Choose a reason for hiding this comment

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

Is this generic enough to have it in resource_action?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is at least true for location and organization, as both are a taxonomy, and the bug linked is affecting all taxonomies

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, i meant whether it can happen on other actions than delete, and whether we should employ it for create, update, you name it.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I wasn't sure about that. and the json this API endpoint returns on errors is slightly different then all the other endpoints, so let's keep it in delete for now.

Copy link
Member

@sean797 sean797 left a comment

Choose a reason for hiding this comment

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

From my view, this is good to merge 👍

@mdellweg ?

@mdellweg mdellweg merged commit 16590ff into theforeman:master Aug 27, 2019
@evgeni evgeni deleted the delete-nested-loc branch September 24, 2019 06:56
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.

3 participants