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 copy - remove ids pointing to the original dialog #5856

Merged
merged 1 commit into from
Jul 25, 2019
Merged

Dialog copy - remove ids pointing to the original dialog #5856

merged 1 commit into from
Jul 25, 2019

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jul 23, 2019

When copying a dialog, we load the original, allow the user to change it, and save that data as a new dialog.

But, the API just saves the fields, groups and tabs, then saves the dialog, and only after that updates those tabs to point to the new dialog. No transactions.
Which means that when dialog validation fails, all the copied tabs still point to the original dialog, leading to duplicate tabs appearing in the original.

This does not fix the API bug or attempt to wrap it in a transaction,
this just makes sure the UI won't send any obsolete ids when copying,
thus limiting the impact of the API bug to extra unused tabs remaining in the db.
(And created ManageIQ/manageiq-api#630.)

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

When copying a dialog, we load the original, allow the user to change it, and save that data as a new dialog.

But, the API just saves the fields, groups and tabs, then saves the dialog, and only after that updates those tabs to point to the new dialog. No transactions.
Which means that when dialog validation fails, all the copied tabs still point to the original dialog, leading to duplicate tabs appearing in the original.

This does not fix the API bug or attempt to wrap it in a transaction,
this just makes sure the UI won't send any obsolete ids when copying,
thus limiting the impact of the API bug to extra unused tabs remaining in the db.

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

miq-bot commented Jul 23, 2019

Checked commit https://github.com/himdel/manageiq-ui-classic/commit/4962568f5586895f95fc7d0b7093737e4d3de041 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

@mzazrivec mzazrivec self-assigned this Jul 25, 2019
@mzazrivec mzazrivec added this to the Sprint 117 Ending Aug 5, 2019 milestone Jul 25, 2019
@mzazrivec mzazrivec merged commit 9c80759 into ManageIQ:master Jul 25, 2019
@himdel himdel deleted the dialog-copy branch July 25, 2019 10:07
simaishi pushed a commit that referenced this pull request Jul 25, 2019
Dialog copy - remove ids pointing to the original dialog

(cherry picked from commit 9c80759)

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

Ivanchuk backport details:

$ git log -1
commit 9e9d63e2ec014989eca31bfaa57f61623c6c790a
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Thu Jul 25 12:04:05 2019 +0200

    Merge pull request #5856 from himdel/dialog-copy
    
    Dialog copy - remove ids pointing to the original dialog
    
    (cherry picked from commit 9c807596ac916a4356662aef891c57589791780e)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1713100

simaishi pushed a commit that referenced this pull request Jul 25, 2019
Dialog copy - remove ids pointing to the original dialog

(cherry picked from commit 9c80759)

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

Hammer backport details:

$ git log -1
commit cda974949fcae6df3fdc54347a76dcdc433cc3c2
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Thu Jul 25 12:04:05 2019 +0200

    Merge pull request #5856 from himdel/dialog-copy
    
    Dialog copy - remove ids pointing to the original dialog
    
    (cherry picked from commit 9c807596ac916a4356662aef891c57589791780e)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1733375

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.

4 participants