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

Fixed issues with 'Back' button on right size screen #5878

Merged
merged 1 commit into from Jul 30, 2019

Conversation

h-kataria
Copy link
Contributor

Multiple issues fixed with back button on non-explorer version of VM Right Size recommendations screen

  • fixed a typo it should be @record.id
  • fixed call to javascript_prologue, method does not expect argument
  • added code to redirect back to previous screen when back button is pressed

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

Note: This is only an issue when drilling down to list of VMs thru relationships and selecting a VM from a list view to view it's right size recommendations.

Please review @hstastna @skateman

@skateman
Copy link
Member

I am okay with the changes, but I think we need a test for it.

@hstastna
Copy link
Contributor

@miq-bot add_reviewer @hstastna

@miq-bot miq-bot requested a review from hstastna July 26, 2019 08:07
Copy link
Contributor

@hstastna hstastna left a comment

Choose a reason for hiding this comment

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

The fix works fine, I've tested also in other screens (not just the one from the BZ) ❇️
And I agree with David that some test would be good to have :)

@h-kataria h-kataria force-pushed the vm_right_size_back_button_fix branch from 3cc063c to 738562f Compare July 26, 2019 20:31
describe "#right_size" do
before do
stub_user(:features => :all)
@vm = FactoryBot.create(:vm_vmware)
Copy link
Member

Choose a reason for hiding this comment

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

Instance variables in rspec can tamper with other specs, please use this outside the before block.

Suggested change
@vm = FactoryBot.create(:vm_vmware)
let!(:vm) { FactoryBot.create(:vm_vmware) }

Multiple issues fixed with back button on non-explorer version of VM Right Size recommendations screen

- fixed a typo it should be `@record.id`
- fixed call to javascript_prologue, method does not expect argument
- added code to redirect back to previous screen when back button is pressed

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1733207
@h-kataria h-kataria force-pushed the vm_right_size_back_button_fix branch from 738562f to 3389bdd Compare July 29, 2019 13:27
@miq-bot
Copy link
Member

miq-bot commented Jul 29, 2019

Checked commit h-kataria@3389bdd with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The Seal of Approval

@mzazrivec mzazrivec added this to the Sprint 117 Ending Aug 5, 2019 milestone Jul 30, 2019
@mzazrivec mzazrivec merged commit a459a48 into ManageIQ:master Jul 30, 2019
simaishi pushed a commit that referenced this pull request Jul 30, 2019
Fixed issues with 'Back' button on right size screen

(cherry picked from commit a459a48)

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

Ivanchuk backport details:

$ git log -1
commit 110c395d10f97d045341b6494d616b327663cf74
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Tue Jul 30 15:06:25 2019 +0200

    Merge pull request #5878 from h-kataria/vm_right_size_back_button_fix
    
    Fixed issues with 'Back' button on right size screen
    
    (cherry picked from commit a459a4823a30b138f44a412b8c10a1f969b1b19a)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1733207

@h-kataria h-kataria deleted the vm_right_size_back_button_fix branch August 1, 2019 17:22
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.

None yet

7 participants