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

Dialog#destroy should ignore resource actions linking nonexisting entities #19574

Merged
merged 1 commit into from
Dec 9, 2019
Merged

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Dec 3, 2019

Currently, if you have a Dialog associated with a Custom Button,
and delete the custom button (as opposed to destroy; that should not happen, but apparently does),

deleting the dialog will fail, because ra.resource_type.constantize.find(ra.resource_id) throws an exception as the resource no longer exists. (The resource being a removed custom button.)

So, updating reject_if_has_resource_action to ignore resource actions pointing to nonexistent resources.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1770300
@miq-bot add_label bug, hammer/yes, ivanchuk/yes


I'm more convinced that this fixes the BZ, than that the problem is something we should fix.
However, for custom buttons, this is not the first time it happened (#19573 and ManageIQ/manageiq-schema#370 being mildly related),
so IMO it makes sense to make sure we don't crash in such situations.

Cc @d-m-u, feel free to disagree ;)

Copy link
Contributor

@d-m-u d-m-u left a comment

Choose a reason for hiding this comment

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

So while conceptually I disagree since I think the issue's entirely self-inflicted, this looks good and I'm fine with it irl.

Thanks for the help, @himdel.

…ities

Currently, if you have a Dialog associated with a Custom Button,
and `delete` the custom button (as opposed to `destroy`; that should not happen, but apparently does),

deleting the dialog will fail, because `ra.resource_type.constantize.find(ra.resource_id)` throws an exception as the resource no longer exists. (The resource being a removed custom button.)

So, updating `reject_if_has_resource_action` to ignore resource actions pointing to nonexistent resources.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1770300
@miq-bot
Copy link
Member

miq-bot commented Dec 3, 2019

Checked commit https://github.com/himdel/manageiq/commit/773a1547fb99d0fb1b91d28ce1f6ce5f53f44490 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 3 offenses detected

app/models/dialog.rb

spec/models/dialog_spec.rb

@h-kataria h-kataria self-assigned this Dec 9, 2019
@h-kataria h-kataria added this to the Sprint 126 Ending Dec 9, 2019 milestone Dec 9, 2019
@h-kataria h-kataria merged commit 79a77cd into ManageIQ:master Dec 9, 2019
@himdel himdel deleted the bz1770300 branch December 9, 2019 17:30
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit a335940e4946974b3dd4420425573851865398b0
Author: Harpreet Kataria <[email protected]>
Date:   Mon Dec 9 11:01:18 2019 -0500

    Merge pull request #19574 from himdel/bz1770300

    Dialog#destroy should ignore resource actions linking nonexisting entities

    (cherry picked from commit 79a77cdcb9ff3254843a5388f23c0020b8a729d4)

    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1783383

simaishi pushed a commit that referenced this pull request Dec 13, 2019
Dialog#destroy should ignore resource actions linking nonexisting entities

(cherry picked from commit 79a77cd)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1783383
@simaishi
Copy link
Contributor

@himdel Travis is failing in hammer branch. Please take a look.

  1) Dialog#destroy does destroy dialog with only obsolete resource_action association
     Failure/Error: FactoryBot.create(:resource_action, :action => nil, :dialog => dialog, :resource_type => 'CustomButton', :resource_id => custom_button.id)
     
     NameError:
       undefined local variable or method `dialog' for #<RSpec::ExampleGroups::Dialog::Destroy:0x000000001e7e8c40>
     # ./spec/models/dialog_spec.rb:124:in `block (3 levels) in <top (required)>'

@himdel
Copy link
Contributor Author

himdel commented Dec 16, 2019

It's because hammer is missing the fixes from #18479, that one is hammer/no, creating a custom PR to add the let(:dialog) bit... #19646

simaishi pushed a commit that referenced this pull request Dec 16, 2019
Dialog#destroy should ignore resource actions linking nonexisting entities

(cherry picked from commit 79a77cd)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1783382
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 613cfaa32b1206ded773049054e17f738999b506
Author: Harpreet Kataria <[email protected]>
Date:   Mon Dec 9 11:01:18 2019 -0500

    Merge pull request #19574 from himdel/bz1770300

    Dialog#destroy should ignore resource actions linking nonexisting entities

    (cherry picked from commit 79a77cdcb9ff3254843a5388f23c0020b8a729d4)

    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1783382

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

Successfully merging this pull request may close these issues.

6 participants