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 custom actions hrefs #252

Merged
merged 1 commit into from
Dec 18, 2017

Conversation

imtayadeway
Copy link
Contributor

Because custom actions are represented as a plain hash in the model,
we lose all context about the type of object we must add an href to in
the normalization process. This process tries, in lieu of this
context, to guess what the href path should look like, and is
incorrectly linking these objects to the parent object's
API (e.g. generic objects, services, etc..). Refactoring all of
normalization to get the proper context will be necessary in order to
fix this correctly. Instead, we intercept the result here, adding the
correct hrefs, which will not be overwritten later.

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

@imtayadeway imtayadeway added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 18, 2017
Copy link
Member

@chrisarcand chrisarcand left a comment

Choose a reason for hiding this comment

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

⚠️ This is the hackiest of hardcoded hacks, for anyone paying attention. Unfortunately, there's really no way to fix this without major, fundamental changes to how the API works that we can think of. Here be dragons. 🔥 🐉

result = {attr => normalize_attr(attr, value)}
# set nil vtype above to "#{type}/#{resource.id}/#{attr}" to support id normalization
[value, result]
end

#
# WARNING: This is a hack.
Copy link
Member

@Fryguy Fryguy Dec 18, 2017

Choose a reason for hiding this comment

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

Prefer HACK: as a lead-in to the comment. HACK: (same with TODO: and FIXME: ) are recognized by a lot of IDEs and tools to allow you to keep a checklist of things to go back to.

See

# HACK: Format attrs to use accepts_nested_attributes_for (Entitlements)
as an example of what I mean.

Copy link
Member

Choose a reason for hiding this comment

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

I love that # HACK: is the official way to do it.

@abellotti
Copy link
Member

@imtayadeway looks good for a HACK 😉

Can you fix the spec rubocop above since that's most likely staying around. And since you're updating, can you update the comment to the "official" HACK: prefix ? Thanks.

Because custom actions are represented as a plain hash in the model,
we lose all context about the type of object we must add an href to in
the normalization process. This process tries, in lieu of this
context, to guess what the href path should look like, and is
incorrectly linking these objects to the parent object's
API (e.g. generic objects, services, etc..). Refactoring all of
normalization to get the proper context will be necessary in order to
fix this correctly. Instead, we intercept the result here, adding the
correct hrefs, which will not be overwritten later.

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

miq-bot commented Dec 18, 2017

Checked commit imtayadeway@76db03e with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@abellotti
Copy link
Member

Thanks @imtayadeway for fixing this 🎵

@abellotti abellotti merged commit 8a3e3cd into ManageIQ:master Dec 18, 2017
simaishi pushed a commit that referenced this pull request Dec 19, 2017
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit c07b0ee4c5cc6e1c7c01dbfe878e1e283e608f93
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Mon Dec 18 17:34:18 2017 -0500

    Merge pull request #252 from imtayadeway/bug/custom-button-hrefs
    
    Fix custom actions hrefs
    (cherry picked from commit 8a3e3cd64225dc585e6d39dd02b7c093542895ec)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1527564

@imtayadeway imtayadeway deleted the bug/custom-button-hrefs branch January 12, 2018 15:43
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.

7 participants