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 missing stylesheet extension #1332

Merged
merged 1 commit into from May 11, 2017

Conversation

@hayesr
Copy link
Contributor

hayesr commented May 11, 2017

Referencing the file without an extension may have worked in older versions of Prince/Rails/etc. Now it seems necessary.

Core PR ManageIQ/manageiq#14793 depends on this change.

Related to this BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1447940

And part of the fixes in #1090

Current look of PDFs

screen shot 2017-05-11 at 11 29 29 am

/cc @dclarizio

@hayesr hayesr force-pushed the hayesr:fix_pdf_stylesheet_ext branch from 446a0ad to 7c50807 May 11, 2017
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented May 11, 2017

Checked commit hayesr@7c50807 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐️

@h-kataria

This comment has been minimized.

Copy link
Contributor

h-kataria commented May 11, 2017

@hayesr can you add before/after screenshots.

@hayesr

This comment has been minimized.

Copy link
Contributor Author

hayesr commented May 11, 2017

@h-kataria I included the current state of PDFs, but this change alone will make no visual difference. Without ManageIQ/manageiq#14793 the path that is sent to Prince is broken and you get the unstyled version. (and without this PR the core PR will result in errors shown in the PDF)

@h-kataria

This comment has been minimized.

Copy link
Contributor

h-kataria commented May 11, 2017

@hayesr thx for the screenshot and info. i will merge this PR and once core PR is merged PDF should look nice and beautiful :-)

@h-kataria h-kataria merged commit 6d21b2e into ManageIQ:master May 11, 2017
3 checks passed
3 checks passed
Hakiri No security warnings were found.
Details
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
simaishi added a commit that referenced this pull request Jun 16, 2017
Fix missing stylesheet extension
(cherry picked from commit 6d21b2e)

https://bugzilla.redhat.com/show_bug.cgi?id=1460815
@simaishi

This comment has been minimized.

Copy link
Contributor

simaishi commented Jun 16, 2017

Fine backport details:

$ git log -1
commit 05b8cd191aaceb294a808d908f10c2bc62d153fd
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Thu May 11 15:59:39 2017 -0400

    Merge pull request #1332 from hayesr/fix_pdf_stylesheet_ext
    
    Fix missing stylesheet extension
    (cherry picked from commit 6d21b2eca5efd704e0da9ede5455eb2779972ecc)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460815
@simaishi simaishi added fine/backported and removed fine/yes labels Jun 16, 2017
@simaishi

This comment has been minimized.

Copy link
Contributor

simaishi commented Jul 7, 2017

Euwe backport (to manageiq repo) details:

$ git log -1
commit b744bb6ce376e7abe972fa45695497ed60d4fd18
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Thu May 11 15:59:39 2017 -0400

    Merge pull request #1332 from hayesr/fix_pdf_stylesheet_ext
    
    Fix missing stylesheet extension
    (cherry picked from commit 6d21b2eca5efd704e0da9ede5455eb2779972ecc)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1465081
@simaishi simaishi added euwe/backported and removed euwe/yes labels Jul 7, 2017
@simaishi

This comment has been minimized.

Copy link
Contributor

simaishi commented Jul 7, 2017

Travis is failing for Euwe branch now... @hayesr @h-kataria would you mind taking a look?

Failures:
  1) EmsInfraController#show download pdf file should match proper title of report
     Failure/Error: pdf_data = PdfGenerator.pdf_from_string(html_string, "pdf_summary.css")
       #<PdfGenerator (class)> received :pdf_from_string with unexpected arguments
         expected: ("", "pdf_summary")
              got: ("", "pdf_summary.css")
        Please stub a default value first if message might be received with other args as well. 
     # ./app/controllers/application_controller/report_downloads.rb:187:in `set_summary_pdf_data'
     # ./app/controllers/ems_common.rb:12:in `show_download'
     # ./app/controllers/ems_common.rb:135:in `show'
     # ./spec/controllers/ems_common_controller_spec.rb:312:in `block (4 levels) in <top (required)>'

  2) EmsInfraController#show download pdf file should not contains string 'ManageIQ' in the title of summary report
     Failure/Error: pdf_data = PdfGenerator.pdf_from_string(html_string, "pdf_summary.css")
       #<PdfGenerator (class)> received :pdf_from_string with unexpected arguments
         expected: ("", "pdf_summary")
              got: ("", "pdf_summary.css")
        Please stub a default value first if message might be received with other args as well. 
     # ./app/controllers/application_controller/report_downloads.rb:187:in `set_summary_pdf_data'
     # ./app/controllers/ems_common.rb:12:in `show_download'
     # ./app/controllers/ems_common.rb:135:in `show'
     # ./spec/controllers/ems_common_controller_spec.rb:312:in `block (4 levels) in <top (required)>'
h-kataria added a commit to h-kataria/manageiq that referenced this pull request Jul 10, 2017
Fixed spec test failure caused by EUWE backport of ManageIQ/manageiq-ui-classic#1332
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.