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

Use record instead of ems instance var in textual authentication status #2823

Merged
merged 1 commit into from
Dec 4, 2017

Conversation

skateman
Copy link
Member

Somehow the container PDF report summary generation is different from the other screens. I'm not sure about the fix, @martinpovolny could you please check?

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

@martinpovolny
Copy link
Member

martinpovolny commented Nov 27, 2017

Generally we want to use @record for the textual summaries so I like the fix.

Question arrise about why the @ems was there. Was that leftover or is this code used in some place where @ems and @record differ?

@skateman
Copy link
Member Author

skateman commented Nov 28, 2017

@martinpovolny the only thing I found was something like @ems = @record = .... Not sure why do we have it here. Checked this on similar pages and the @record has been used everywhere.

@martinpovolny
Copy link
Member

Ok. Please, write a simple spec that will call the stuff used for PDF generation and will reach this method. Thx!

@skateman skateman force-pushed the container-pdf-report branch 2 times, most recently from dbbb16e to a554437 Compare November 30, 2017 12:39
@skateman
Copy link
Member Author

@martinpovolny the PDF generation is not available in the testing environment and mocking the generator would just make the test meaningless. Do you have any ideas?

@martinpovolny
Copy link
Member

Well the PDF generator is fed with some HTML. So just call that HTML rendering the way it's called when the PDF is generated and assert on top of that HTML. Does that make sense?

@skateman skateman force-pushed the container-pdf-report branch 2 times, most recently from 19ca41c to 3f3d11f Compare November 30, 2017 15:44
@martinpovolny
Copy link
Member

:exclamation: - Line 256, Col 7 - Rails/HttpPositionalArguments - Use keyword arguments instead of positional arguments for http call: get.

this?

@skateman skateman force-pushed the container-pdf-report branch from 3f3d11f to 17028e9 Compare November 30, 2017 16:03
@skateman skateman force-pushed the container-pdf-report branch from 17028e9 to a6f3450 Compare November 30, 2017 16:44
@miq-bot
Copy link
Member

miq-bot commented Nov 30, 2017

Checked commit skateman@a6f3450 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

app/helpers/textual_mixins/textual_authentications_status.rb

@martinpovolny martinpovolny merged commit f83b8f6 into ManageIQ:master Dec 4, 2017
@martinpovolny martinpovolny added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 4, 2017
@martinpovolny martinpovolny self-assigned this Dec 4, 2017
@skateman skateman deleted the container-pdf-report branch December 4, 2017 14:00
simaishi pushed a commit that referenced this pull request Dec 4, 2017
Use record instead of ems instance var in textual authentication status
(cherry picked from commit f83b8f6)

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

simaishi commented Dec 4, 2017

Gaprindashvili backport details:

$ git log -1
commit b3b454801f965934555d0da6270b02dd14c9ee49
Author: Martin Povolny <[email protected]>
Date:   Mon Dec 4 14:53:55 2017 +0100

    Merge pull request #2823 from skateman/container-pdf-report
    
    Use record instead of ems instance var in textual authentication status
    (cherry picked from commit f83b8f6ba930f38d51fb83ff12a1af12b1fd61a1)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1520531

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