Fix issues related to adding, editing Chargeback Rates #3129

hstastna · 2017-12-21T12:26:17Z

Fix issues related to adding new or editing existing chargeback rates in Cloud Intel -> Chargeback
-> Rates tab, caused by improper detail_index in the chargeback rate edit table.

Issues:

on mouse focus, the whole form/table is highlighted instead of one row, when adding a new chargeback rate
https://user-images.githubusercontent.com/14937244/34208255-0446729a-e58e-11e7-8e4a-ecaadf5b419b.gif
if a new tier is added (by pressing Add button in the table), the then tier is added to the improper metric
https://user-images.githubusercontent.com/14937244/34208319-56445364-e58e-11e7-803b-d0c4673727ab.gif

After:

Details:
Simple condition was added to haml, because it is accessed not only if editing chargeback rate, but also if adding a new one. We don't need editing detail_index if we just add a new rate.

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

hstastna · 2017-12-21T12:27:31Z

@miq-bot add_label bug, gaprindashvili/yes

romanblanco

Fixes both issues described in ManageIQ/manageiq#16699. 👍

romanblanco · 2018-01-05T13:32:49Z

Tests are passing locally.

fixing ManageIQ/manageiq#16699 Fix issues with adding new or editing existing chargeback rates in Cloud Intel -> Chargeback -> Rates tab, caused by improper detail_index in the chargeback rate edit table.

hstastna · 2018-01-09T13:37:05Z

@himdel @martinpovolny It looks like this could be merged. If any issues occur, please just let me know. Thanks!

miq-bot · 2018-01-09T13:37:24Z

Checked commit hstastna@97303e5 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

app/views/chargeback/_cb_rate_edit_table.html.haml

⚠️ - Line 21 - Block has too many lines. [63/25]

lpichler · 2018-01-10T13:18:47Z

@hstastna please reopen PR to restart travis.

@ZitaNemeckova is this PR fixing your BZ?

ZitaNemeckova · 2018-01-10T13:25:40Z

@lpichler Yeah it is :)

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

@hstastna Can you please close and open this PR? Travis failures are not related to your changes and it should go green :)

ZitaNemeckova · 2018-01-10T15:46:33Z

This fixes blocker bz https://bugzilla.redhat.com/show_bug.cgi?id=1532368 so adding label :)
@miq-bot add_label blocker

h-kataria · 2018-01-10T16:38:28Z

@ZitaNemeckova @hstastna please review changes that were made in 36163ea to make variable rate field non-editable.

himdel · 2018-01-10T16:47:24Z

app/views/chargeback/_cb_rate_edit_table.html.haml

      - @cur_group = detail[:group] if @cur_group.nil?
      - if @cur_group != detail[:group]
        - @cur_group = detail[:group]
        %tr
          %td{:colspan => "10", :style => "background-color: #f5f5f5;"} &nbsp;
-      - detail_index = @edit[:new][:tiers].index { |x| x.detect { |tier_hash| tier_hash['chargeback_rate_detail_id'] == detail[:id] }.present? }
+      - if params[:pressed] == "chargeback_rates_edit"


This doesn't sound like the best place for this kind of condition.

Also, this means detail_index will be nil for all the other cases...

So.. all the code like {:id => "rate_detail_row_#{detail_index}_0"} has a bug now. (Well, unless nil is really expected there.)

Please make sure detail_index always has a reasonable value.
(And ideally, move the params[:pressed] condition to controller code.)

detail_index will not be nil because of https://github.com/ManageIQ/manageiq-ui-classic/pull/3129/files#diff-9e228e37bdb4a0dfb3c30cffd92d3981R21
....each_with_index do |detail, detail_index|
I have tested that, it always will be set, and properly. detail_index is just changed if params[:pressed] == "chargeback_rates_edit"
Or am I missing something?
This change is made because in this haml... this haml is common for editing and displaying the rates. For just displaying, using detail_index just as it is, works perfectly. When editing, we need it just like this: https://github.com/ManageIQ/manageiq-ui-classic/pull/3129/files#diff-9e228e37bdb4a0dfb3c30cffd92d3981L27
To move the condition to controller? I am not sure, how I would change detail_index from controller if the rate is edited. Any idea how to make it properly? Thanks! :)

ZitaNemeckova · 2018-01-10T16:49:45Z

@h-kataria Yeah but only for fixed ones.

dclarizio · 2018-01-10T23:21:16Z

@lpichler can you please review and let us know if this is good to go? Thx, Dan

hstastna · 2018-01-11T11:24:26Z

@h-kataria I think that 36163ea is ok. I don't see how this could affect this PR. The commit change is already in the code and I was fixing the issue against the actual code.

lpichler

@dclarizio tested and it is working as expected 👍

@himdel
detail_index is going from as an index from
.each_with_index do |detail, detail_index| as @hstastna is saying but for editing we need to recalculated index when the rate is edited.

This doesn't sound like the best place for this kind of condition

yes, it doesn't but there is more similar array operations in the haml a maybe all these should live somewhere else, maybe in the helper, maybe it would be solved by some cleanup's in the controller.

I am considering this solution as most safe what we can do now and I am suggesting to solve these technical debts in follow up PR. There is also probabilty that this section would be replaced by something 'new' in future.

…per_ids Fix issues related to adding, editing Chargeback Rates (cherry picked from commit 5cd68c7) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1533924

simaishi · 2018-01-12T14:44:29Z

Gaprindashvili backport details:

$ git log -1
commit db13d23fd1e0384ce9d4527c47f41606a9185eab
Author: Dan Clarizio <dclarizi@redhat.com>
Date:   Thu Jan 11 08:38:48 2018 -0800

    Merge pull request #3129 from hstastna/Form_add_chargeback_rate_improper_ids
    
    Fix issues related to adding, editing Chargeback Rates
    (cherry picked from commit 5cd68c7eda217b5fa10fe1021f68122a3a68399c)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1533924

hstastna mentioned this pull request Dec 21, 2017

Form add new chargeback rate using improper ids ManageIQ/manageiq#16699

Closed

miq-bot added bug gaprindashvili/yes labels Dec 21, 2017

hstastna changed the title ~~Fix issues with adding, editing Chargeback Rates~~ Fix issues related to adding, editing Chargeback Rates Dec 21, 2017

hstastna force-pushed the Form_add_chargeback_rate_improper_ids branch from b727941 to c0e8ed8 Compare December 21, 2017 14:08

hstastna force-pushed the Form_add_chargeback_rate_improper_ids branch from c0e8ed8 to a2adf5c Compare January 2, 2018 13:27

romanblanco approved these changes Jan 5, 2018

View reviewed changes

himdel closed this Jan 5, 2018

himdel reopened this Jan 5, 2018

Fix issues with adding or editing chargeback rates

97303e5

fixing ManageIQ/manageiq#16699 Fix issues with adding new or editing existing chargeback rates in Cloud Intel -> Chargeback -> Rates tab, caused by improper detail_index in the chargeback rate edit table.

hstastna force-pushed the Form_add_chargeback_rate_improper_ids branch from a2adf5c to 97303e5 Compare January 9, 2018 13:34

hstastna closed this Jan 10, 2018

hstastna reopened this Jan 10, 2018

miq-bot added the blocker label Jan 10, 2018

himdel reviewed Jan 10, 2018

View reviewed changes

dclarizio added the overview/chargeback label Jan 10, 2018

dclarizio self-assigned this Jan 10, 2018

lpichler approved these changes Jan 11, 2018

View reviewed changes

dclarizio merged commit 5cd68c7 into ManageIQ:master Jan 11, 2018

dclarizio added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 11, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues related to adding, editing Chargeback Rates #3129

Fix issues related to adding, editing Chargeback Rates #3129

hstastna commented Dec 21, 2017 •

edited by simaishi

hstastna commented Dec 21, 2017

romanblanco left a comment

romanblanco commented Jan 5, 2018

hstastna commented Jan 9, 2018

miq-bot commented Jan 9, 2018

lpichler commented Jan 10, 2018

ZitaNemeckova commented Jan 10, 2018

ZitaNemeckova commented Jan 10, 2018

h-kataria commented Jan 10, 2018

himdel Jan 10, 2018 •

edited

hstastna Jan 10, 2018 •

edited

ZitaNemeckova commented Jan 10, 2018

dclarizio commented Jan 10, 2018

hstastna commented Jan 11, 2018

lpichler left a comment •

edited

simaishi commented Jan 12, 2018

Fix issues related to adding, editing Chargeback Rates #3129

Fix issues related to adding, editing Chargeback Rates #3129

Conversation

hstastna commented Dec 21, 2017 • edited by simaishi

hstastna commented Dec 21, 2017

romanblanco left a comment

Choose a reason for hiding this comment

romanblanco commented Jan 5, 2018

hstastna commented Jan 9, 2018

miq-bot commented Jan 9, 2018

lpichler commented Jan 10, 2018

ZitaNemeckova commented Jan 10, 2018

ZitaNemeckova commented Jan 10, 2018

h-kataria commented Jan 10, 2018

himdel Jan 10, 2018 • edited

Choose a reason for hiding this comment

hstastna Jan 10, 2018 • edited

Choose a reason for hiding this comment

ZitaNemeckova commented Jan 10, 2018

dclarizio commented Jan 10, 2018

hstastna commented Jan 11, 2018

lpichler left a comment • edited

Choose a reason for hiding this comment

simaishi commented Jan 12, 2018

hstastna commented Dec 21, 2017 •

edited by simaishi

himdel Jan 10, 2018 •

edited

hstastna Jan 10, 2018 •

edited

lpichler left a comment •

edited