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

Prevent IR request link from appearing on all interstitial pages #310

Merged
merged 28 commits into from Sep 30, 2020

Conversation

petechd
Copy link
Contributor

@petechd petechd commented Sep 28, 2020

What is the context of this PR?

This change prevents individual response link from appearing on all interstitial pages. Trello card.

How to review

Manually run test_individual_response schema to check if this change works, use test_individual_response.py integration test for automated testing.

Checklist

  • New static content marked up for translation
  • Newly defined schema content included in eq-translations repo

@petechd
Copy link
Contributor Author

petechd commented Sep 28, 2020

templates/interstitial.html Outdated Show resolved Hide resolved
app/views/handlers/__init__.py Outdated Show resolved Hide resolved
app/views/handlers/__init__.py Outdated Show resolved Hide resolved
Comment on lines 37 to 39
"show_individual_response_link": show_individual_response_link(
self._current_location, self._schema
)
Copy link
Contributor

@iwootten iwootten Sep 28, 2020

Choose a reason for hiding this comment

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

Currently there's a lot of duplication here.

I would expect all the templates that show the IR link (hub/interstitial) to use the same variable for determining visibility of the link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duplication removed, extra variable gone. Just need to add tests now👍.

Copy link
Contributor

@MebinAbraham MebinAbraham left a comment

Choose a reason for hiding this comment

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

Would need to add a test to ensure this bug doesn't resurface,


@staticmethod
def show_individual_response(location, schema):
section = schema.json.get("individual_response", {}).get(
Copy link
Contributor

@iwootten iwootten Sep 29, 2020

Choose a reason for hiding this comment

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

I think you've misunderstood my original comment here - the first line here that returns the section_id should be a separate method I think and shouldn't be static when part of questionnaire_schema as schema is an instance of it. I think this method name should probably change too as it doesn't describe the check. Something like is_individual_response_interstitial makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

For this method, is_first_block_in_individual_response is probably more appropriate since it being an interstitial is irrelevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iwootten that first line is now a method in Questionnaire Schema class. Thanks!

Comment on lines 501 to 504
def get_individual_response_individual_section_id(self):
return self._questionnaire_json.get("individual_response", {}).get(
"individual_section_id"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to group this with the other IR methods in this class for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to ir group now.

petechd and others added 2 commits September 30, 2020 09:52
Co-authored-by: Mebin Abraham <35296336+MebinAbraham@users.noreply.github.com>
@petechd petechd merged commit 5a936f1 into master Sep 30, 2020
@petechd petechd deleted the fix-request-link branch September 30, 2020 09:59
@pricem14pc pricem14pc added this to the v3.50.0 milestone Oct 6, 2020
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.

None yet

4 participants