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

fixes for downloads #1170

Merged
merged 1 commit into from
Feb 16, 2018
Merged

fixes for downloads #1170

merged 1 commit into from
Feb 16, 2018

Conversation

briri
Copy link
Contributor

@briri briri commented Feb 15, 2018

addresses issues in #1127

  • removed .joins(:answers) since we cannot guarantee that the plan has at least one answer before user tries to download.
  • also added check for blank answer before trying to access its options
  • updated policy for organisationally visible plan download so that it compares the plan owner's org instead of the template creator's org Error message on downloading PDFs of shared institutional plans #1126

updated security check
@@ -43,7 +43,7 @@
<% blank = answer.present? ? answer.text.gsub(/<\/?p>/, '').gsub(/<br\s?\/?>/, '').chomp.blank? : true %>
Copy link
Contributor

Choose a reason for hiding this comment

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

using answer.is_valid? would help to simplify this context. we should address this later.

Copy link
Contributor Author

@briri briri Feb 16, 2018

Choose a reason for hiding this comment

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

ah yeah. adding the code to ignore the html to this is_valid would be better.
Could probably make use of the Nokogiri gem to load the answer.text value into html and then do a check on that html object's .text() to make sure its ignoring all markup when checking the answer. Instead of doing the regex matches

@@ -43,7 +43,7 @@
<% blank = answer.present? ? answer.text.gsub(/<\/?p>/, '').gsub(/<br\s?\/?>/, '').chomp.blank? : true %>
<% if blank && @show_unanswered %>
<p><%= _('Question not answered.') -%></p>
<% else %>
<% elsif !blank %>
Copy link
Contributor

Choose a reason for hiding this comment

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

rails helper method .present? would be a clearer solution instead of the baked variable blank we have defined above...

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 would be but I'm seeing a lot of <p></p> stored in the DB. I'm thinking that this is happening when the user blanks out the answer. That value always equates to present? true

Copy link
Contributor

Choose a reason for hiding this comment

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

yep! If that's the case we should look at tinymce to clear out everything so the answer text becomes '' and therefore .present? is false

@jollopre jollopre merged commit a73119a into DMPRoadmap:development Feb 16, 2018
@briri briri deleted the issue1127 branch February 21, 2018 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants