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

Fix miq request ownership #18257

Merged
merged 2 commits into from
Dec 3, 2018
Merged

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Dec 3, 2018

Overview

Currently, MiqRequests only show requests created in the same region.
This works most of the time except for a rollup (aka reporting) region.

You see, we have different user records for each region. They have the same userid (aka the string login name), but the id is different.
Requests created in this region are associated with the user from this region, and requests created in another region are associated with the user from the other region.

Followup to #18245 (added specs)
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1647169

Before

For the request screen, we only showed requests associated with the currently logged in user.
Requests from other regions were not associated with this user and did not show.

After

For the request screen, we now only show requests associated with the currently logged in userid (the string you use to login).
Requests from other regions are associated with a different user, but the userid match, so they are now shown.

The OwnershipMixin uses userid to store object ownership.
The MiqRequest needed to overrides these methods since ownership is stored in requester_id.
The new methods have been changed to still be based upon requester_id, but the code is now more similar to the OwnershipMixin version.

@kbrock kbrock force-pushed the fix-miq_request-ownership branch from 1cc1575 to b1f7a3d Compare December 3, 2018 19:08
@miq-bot
Copy link
Member

miq-bot commented Dec 3, 2018

Checked commits kbrock/manageiq@c7d2cdc~...b1f7a3d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 🍪

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for taking care of the @kbrock

@gtanzillo gtanzillo added this to the Sprint 100 Ending Dec 3, 2018 milestone Dec 3, 2018
@gtanzillo gtanzillo merged commit aa6fba5 into ManageIQ:master Dec 3, 2018
@kbrock kbrock deleted the fix-miq_request-ownership branch December 3, 2018 20:02
@simaishi
Copy link
Contributor

simaishi commented Dec 4, 2018

@kbrock @gtanzillo hammer/yes and gaprindashvili/yes?

Copy link
Contributor

@lpichler lpichler left a comment

Choose a reason for hiding this comment

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

👍 LGTM, thanks @kbrock

making a note to update factories in these specs when this(Support more regions in FactoryGirl's factories) will be merged (and other PR
s)

simaishi pushed a commit that referenced this pull request Dec 6, 2018
@simaishi
Copy link
Contributor

simaishi commented Dec 6, 2018

Hammer backport details:

$ git log -1
commit 740e6b551760f071217fe25b5ad1296a2a3d9d37
Author: Gregg Tanzillo <[email protected]>
Date:   Mon Dec 3 14:50:54 2018 -0500

    Merge pull request #18257 from kbrock/fix-miq_request-ownership
    
    Fix miq request ownership
    
    (cherry picked from commit aa6fba547ce4fc19e58869e282f836eeb08c44ec)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1647169

simaishi pushed a commit that referenced this pull request Dec 14, 2018
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit dc417f38350157a5bba708d03f45ddda98e403c4
Author: Gregg Tanzillo <[email protected]>
Date:   Mon Dec 3 14:50:54 2018 -0500

    Merge pull request #18257 from kbrock/fix-miq_request-ownership
    
    Fix miq request ownership
    
    (cherry picked from commit aa6fba547ce4fc19e58869e282f836eeb08c44ec)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1656170

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.

5 participants