Fix deletion of Default Container Image Rate #3215

hstastna · 2018-01-10T13:27:10Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1486658
Depends on ManageIQ/manageiq#16792 [MERGED]

Fix deletion of Default Container Image Rate in Cloud Intel -> Chargeback
-> Rates so that the Rate can't be deleted and error message is displayed.

Also the spec tests were added to this PR.

Explanation:
Originally, checking if the selected rate is default (before its deletion), did not work well only if deleting rate(s) from a list of the rates. This was because of missing part of the condition for checking default rates here:
https://github.com/ManageIQ/manageiq/blob/master/app/models/chargeback_rate.rb#L153
Checking if the rate is default, worked only from rate details page. This was because of
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/application_helper/button/chargeback_rate_remove.rb#L12
So I added the missing part of the condition here: https://github.com/ManageIQ/manageiq/compare/master...hstastna:Default_Container_Image_Rate_delete_description?expand=1#diff-7c998acd3d11c70ed830e216754f4b6bR153
I also edited cb_rates_delete method in chargeback controller (in this PR) to be able to properly delete nondefault rates and also to show error messages about deleting default rates - in a one step! I simplified some parts of the method to prevent issues with rendering flash messages in some special cases (if no rates were selected or if the rate no longer exists) or with showing the rates list after deleting the rates (if some of the all selected rates were successfully deleted but some of them not - and this is why I had to remove if-else condition at the end of cb_rates_delete method and to change render_flash to add_flash in the method to prevent multiple rendering issues.. this also required properly setting x_node if deleting a rate from its details page - to properly show the list of remaining rates after the deletion, including the error messages, if there are some).

The bug is related to Compute Chargeback rates.

hstastna · 2018-01-10T13:28:44Z

@miq-bot add_label bug, gaprindashvili/yes

hstastna · 2018-01-24T14:15:01Z

@gtanzillo @h-kataria @zeari @lpichler Could you check also this PR, if you will have some time? :) This PR is related to ManageIQ/manageiq#16792 which is already merged. The bug https://bugzilla.redhat.com/show_bug.cgi?id=1486658 will be completely fixed only after reviewing and merging also this PR. Merging only ManageIQ/manageiq#16792 does not fix the bug completely. Thanks!

h-kataria · 2018-01-24T15:05:13Z

app/controllers/chargeback_controller.rb

+
+    cb_rates_list
+    @right_cell_text = _("%<typ>s Chargeback Rates") % {:typ => x_node.split('-').last}
+    replace_right_cell(:replace_trees => [:cb_rates])


@hstastna i have not tested this in UI but wondering why this was moved out of if/else block, i don't think we need to call cb_rates_list and replace_right_cell methods if there were flash errors such as "no records selected for deletion" in that case we should simply render flash message partial.

As I remember, this was the best option to fix another issues (and to not break anything else) which came with fixing the bug: I came to the situation when I needed to render flash message if some error occurs AND to display remaining rates properly (which were not deleted or which shouldn't have been deleted). Because if we check more than one rate in the list and try to delete them, some of them are successfully deleted and some of them not (like Default rates) and we need to show error message AND the actual list of the rates (if/else block did not allow us to do it properly). We need to update the list after deleting. It was not the best that the if/else block came after deleting the rates. So this PR also allows us to delete the rates which can be deleted and to display error message if there was some error during deletion (for example if one rate was not able to be deleted, like default rate).

h-kataria · 2018-01-24T15:06:51Z

app/controllers/chargeback_controller.rb

      end
    else # showing 1 rate, delete it
-      cb_rate = ChargebackRate.find_by_id(params[:id])
+      cb_rate = ChargebackRate.find_by(:id => params[:id])
+      self.x_node = x_node.split('_').first


@hstastna same here, i think we don't need to reset value of x_node if cb_rate is nil

I needed this for cb_rates_list and replace_right_cell methods which come after this block.

h-kataria · 2018-01-25T04:06:03Z

@hstastna verified in UI, you think you can add spec test around this?

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1486658 Fix deletion of Default Container Image Rate in Cloud Intel -> Chargeback -> Rates so that the Rate can't be deleted and error message is displayed.

Add a spec test for fix for deletion of Default Container Image Rate in Cloud Intel -> Chargeback -> Rates so that the Rate can't be deleted and error message is displayed. The test covers all of the possibilities of deleting the rate (from the list or details page) when cb_rates_list method should be called (which was not like that before the fix). It also checks setting the right cell text which sometimes can cause another errors if it is not set properly. The test is extensible.

miq-bot · 2018-01-30T18:01:29Z

Checked commits hstastna/manageiq-ui-classic@8b05f8b~...821c3d5 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

hstastna · 2018-02-09T12:06:21Z

@h-kataria @dclarizio This is ready to merge, I think :)

…elete Fix deletion of Default Container Image Rate (cherry picked from commit 8806d61) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1552260

simaishi · 2018-03-08T12:47:07Z

Gaprindashvili backport details:

$ git log -1
commit 67174afb2dd94d7443fff3917681998e056b0f4e
Author: Harpreet Kataria <[email protected]>
Date:   Fri Feb 9 11:17:10 2018 -0500

    Merge pull request #3215 from hstastna/Default_Container_Image_Rate_delete
    
    Fix deletion of Default Container Image Rate
    (cherry picked from commit 8806d612ebba610d3319575fd42f4866fcb37808)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1552260

hstastna · 2018-04-06T09:38:31Z

@miq-bot add_label test

simaishi · 2018-04-12T22:35:33Z

@hstastna Can this and the dependent PR ManageIQ/manageiq#16792 be fine/yes?

hstastna · 2018-04-13T07:21:46Z

@simaishi I'm not sure. It depends on flags or target release in the BZ. Doesn't it?

simaishi · 2018-04-13T10:28:48Z

@hstastna The BZ has the 5.8.z flag, which is why I asked 😄 I just can't tell if you need to create a separate PR or that PRs can just be fine/yes and be backported...

hstastna · 2018-04-13T11:01:40Z

@simaishi I can't tell, too. Today I'm on sickday and I have access only to emails, github. Let's try to backport it and if some complicated conflicts occur, I will create a new PR asap.

hstastna · 2018-04-13T11:01:54Z

@miq-bot add_label fine/yes

…elete Fix deletion of Default Container Image Rate (cherry picked from commit 8806d61) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1568084

simaishi · 2018-04-18T17:59:29Z

Fine backport details:

$ git log -1
commit 26cd4f7da686ca9ad212c4e66bca8b7902c20641
Author: Harpreet Kataria <[email protected]>
Date:   Fri Feb 9 11:17:10 2018 -0500

    Merge pull request #3215 from hstastna/Default_Container_Image_Rate_delete
    
    Fix deletion of Default Container Image Rate
    (cherry picked from commit 8806d612ebba610d3319575fd42f4866fcb37808)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1568084

miq-bot added bug gaprindashvili/yes labels Jan 10, 2018

hstastna force-pushed the Default_Container_Image_Rate_delete branch from d547bed to 5b993ef Compare January 10, 2018 13:31

miq-bot added the wip label Jan 10, 2018

hstastna force-pushed the Default_Container_Image_Rate_delete branch 2 times, most recently from 357d30e to 52574c2 Compare January 10, 2018 15:51

hstastna mentioned this pull request Jan 10, 2018

Add condition to fix deletion of Default Container Image Rate ManageIQ/manageiq#16792

Merged

hstastna force-pushed the Default_Container_Image_Rate_delete branch 2 times, most recently from 6d281ac to 994031a Compare January 10, 2018 17:41

hstastna changed the title ~~[WIP] Fix deletion of Default Container Image Rate~~ Fix deletion of Default Container Image Rate Jan 10, 2018

miq-bot removed the wip label Jan 10, 2018

hstastna changed the title ~~Fix deletion of Default Container Image Rate~~ [WIP] Fix deletion of Default Container Image Rate Jan 10, 2018

miq-bot added the wip label Jan 10, 2018

hstastna changed the title ~~[WIP] Fix deletion of Default Container Image Rate~~ Fix deletion of Default Container Image Rate Jan 24, 2018

miq-bot removed the wip label Jan 24, 2018

h-kataria reviewed Jan 24, 2018

View reviewed changes

Hilda Stastna added 2 commits January 30, 2018 16:40

Fix deletion of Default Container Image Rate

8b05f8b

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1486658 Fix deletion of Default Container Image Rate in Cloud Intel -> Chargeback -> Rates so that the Rate can't be deleted and error message is displayed.

hstastna force-pushed the Default_Container_Image_Rate_delete branch from 994031a to 821c3d5 Compare January 30, 2018 17:51

h-kataria self-assigned this Feb 9, 2018

h-kataria added the overview/chargeback label Feb 9, 2018

h-kataria added this to the Sprint 79 Ending Feb 12, 2018 milestone Feb 9, 2018

h-kataria merged commit 8806d61 into ManageIQ:master Feb 9, 2018

hstastna mentioned this pull request Feb 21, 2018

Fix Chargeback Rates page title #3447

Merged

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Mar 8, 2018

miq-bot added the test label Apr 6, 2018

miq-bot added the fine/yes label Apr 13, 2018

simaishi added fine/backported and removed fine/yes labels Apr 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deletion of Default Container Image Rate #3215

Fix deletion of Default Container Image Rate #3215

hstastna commented Jan 10, 2018 •

edited

Loading

hstastna commented Jan 10, 2018

hstastna commented Jan 24, 2018 •

edited

Loading

h-kataria Jan 24, 2018

hstastna Jan 24, 2018 •

edited

Loading

h-kataria Jan 24, 2018

hstastna Jan 24, 2018

h-kataria commented Jan 25, 2018

miq-bot commented Jan 30, 2018

hstastna commented Feb 9, 2018

simaishi commented Mar 8, 2018

hstastna commented Apr 6, 2018

simaishi commented Apr 12, 2018

hstastna commented Apr 13, 2018

simaishi commented Apr 13, 2018

hstastna commented Apr 13, 2018

hstastna commented Apr 13, 2018

simaishi commented Apr 18, 2018

Fix deletion of Default Container Image Rate #3215

Fix deletion of Default Container Image Rate #3215

Conversation

hstastna commented Jan 10, 2018 • edited Loading

hstastna commented Jan 10, 2018

hstastna commented Jan 24, 2018 • edited Loading

h-kataria Jan 24, 2018

Choose a reason for hiding this comment

hstastna Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

h-kataria Jan 24, 2018

Choose a reason for hiding this comment

hstastna Jan 24, 2018

Choose a reason for hiding this comment

h-kataria commented Jan 25, 2018

miq-bot commented Jan 30, 2018

hstastna commented Feb 9, 2018

simaishi commented Mar 8, 2018

hstastna commented Apr 6, 2018

simaishi commented Apr 12, 2018

hstastna commented Apr 13, 2018

simaishi commented Apr 13, 2018

hstastna commented Apr 13, 2018

hstastna commented Apr 13, 2018

simaishi commented Apr 18, 2018

hstastna commented Jan 10, 2018 •

edited

Loading

hstastna commented Jan 24, 2018 •

edited

Loading

hstastna Jan 24, 2018 •

edited

Loading