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.

Sign up for GitHub

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

Jump to bottom

Add template methods needed for provision report #17884

Merged
merged 1 commit into from
Oct 30, 2018
Merged

Add template methods needed for provision report #17884

merged 1 commit into from
Oct 30, 2018

Conversation

PanSpagetka
Copy link
Contributor

@PanSpagetka PanSpagetka mentioned this pull request Aug 21, 2018
Merged
@PanSpagetka PanSpagetka force-pushed the add-template-provision-report-methods branch from 88cc5ed to 534a1bf Compare August 22, 2018 08:03
@PanSpagetka
Copy link
Contributor Author

@lpichler

@miq-bot
Copy link
Member

miq-bot commented Aug 22, 2018

Checked commit PanSpagetka@534a1bf with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@PanSpagetka
Copy link
Contributor Author

@miq-bot add_label hammer/yes

lpichler
lpichler reviewed Oct 1, 2018
View reviewed changes
app/models/miq_template.rb
if respond_to?(:volume_template?) || respond_to?(:volume_snapshot_template?)
_("N/A")
elsif respond_to?(:deprecated)
_(deprecated.to_s)
Copy link
Contributor

Choose a reason for hiding this comment

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

@mzazrivec is this ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, as long as deprecated.to_s is already tagged somewhere for string collection. Otherwise this won't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is true/false string and it was used before in Views. So i think it should be ok.

@PanSpagetka
Copy link
Contributor Author

@h-kataria do you know if someone else would like to review these changes?

@JPrause
Copy link
Member

JPrause commented Oct 11, 2018

@miq-bot add_label blocker

@JPrause
Copy link
Member

JPrause commented Oct 25, 2018

@PanSpagetka as this for a blocker issue, do you have an update on this PR.

@PanSpagetka
Copy link
Contributor Author

ping @martinpovolny

@martinpovolny martinpovolny self-assigned this Oct 30, 2018
@martinpovolny martinpovolny merged commit 67fd66a into ManageIQ:master Oct 30, 2018
@martinpovolny martinpovolny added this to the Sprint 98 Ending Nov 5, 2018 milestone Oct 30, 2018
simaishi pushed a commit that referenced this pull request Oct 31, 2018
@martinpovolny @simaishi
Merge pull request #17884 from PanSpagetka/add-template-provision-rep…
2b4a340 
…ort-methods

Add template methods needed for provision report

(cherry picked from commit 67fd66a)

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

Hammer backport details:

$ git log -1
commit 2b4a340fb31b52e62d7e24a088e3244e15116e6b
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Tue Oct 30 16:38:55 2018 +0100

    Merge pull request #17884 from PanSpagetka/add-template-provision-report-methods
    
    Add template methods needed for provision report
    
    (cherry picked from commit 67fd66a8b9bcb7856b8a684ee5a2452ba0f367d5)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1610927

@himdel
Copy link
Contributor

himdel commented Nov 14, 2018 •
edited
Loading

This will break the moment we actually add those methods.

Should we not be actually calling volume_template? (and the rest) instead of just checking it's there?

(Especially since given those names, those methods should exist on the base class, so that they're always available.)

@martinpovolny
Copy link
Member

@himdel, @PanSpagetka, @lpichler : From the last comment it seems there's some unresolved issue in this PR.

Can you, please, take a look into it?

@himdel
Copy link
Contributor

himdel commented Nov 15, 2018 •
edited
Loading

Everybody ... this is even more problematic :(....

Ruport is using those display_* methods to get the right values.
Which would be fine, except you also need to sort by those fields.
Aaand sorting breaks horribly when you have different types for the same fields (like, "n/a" versus integers)

See ManageIQ/manageiq-ui-classic#4509 (comment) for details.

EDIT: confirmed none of the fields changed here are used in the regular Instances or Templates view in the UI, so this does not break those views. It may still break reports (when sorting by such fields), and definitely breaks in the provisioning screen where the columns are used (but which was not using Ruport before ManageIQ/manageiq-ui-classic#4509). So.. the 2 PRs are mutually exclusive now :)

(EDIT2: note that just doing .to_s on the numbers will fix the exception .. but will no longer sort numerically, so 1000 < 2)

EDIT3: Fixed in #18217 :)

@PanSpagetka PanSpagetka mentioned this pull request Nov 19, 2018
Merged
@PanSpagetka PanSpagetka mentioned this pull request Nov 28, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mzazrivec mzazrivec mzazrivec left review comments

@lpichler lpichler lpichler approved these changes

Assignees

@martinpovolny martinpovolny

Labels
blocker hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@PanSpagetka @miq-bot @JPrause @simaishi @himdel @martinpovolny @mzazrivec @lpichler