-
Notifications
You must be signed in to change notification settings - Fork 7
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
AP-5592: Update scope limits on check provider answers #7541
Conversation
d843e30
to
8caa09a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Just some suggestions for refactoring that big method scope_limitation_details
, which would be good, but non-blocker
@@ -21,10 +21,16 @@ def scope_limits(proceeding, scope_type) | |||
private | |||
|
|||
def scope_limitation_details(scope_limitation) | |||
scope_limitation_details = [scope_limitation.meaning, scope_limitation.description] | |||
sole_scope_limitation = scope_limitation.proceeding.scope_limitations.where(scope_type: scope_limitation.scope_type).count.eql?(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could extract to own method
def sole_scope_limitation?(scope_limitation)
scope_limitation.proceeding.scope_limitations.where(scope_type: scope_limitation.scope_type).count.eql?(1)
end
scope_limitation_meaning = if sole_scope_limitation | ||
scope_limitation.meaning | ||
else | ||
"<span class=\"single-scope-limit-heading\">#{scope_limitation.meaning}</span>".html_safe | ||
end | ||
scope_limitation_details = [scope_limitation_meaning, scope_limitation.description] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another method extraction perhaps?
def scope_limitation_base_details(scope_limitation)
scope_limitation_meaning = if sole_scope_limitation?(scope_limitation)
scope_limitation.meaning
else
"<span class=\"single-scope-limit-heading\">#{scope_limitation.meaning}</span>".html_safe
end
[scope_limitation_meaning, scope_limitation.description]
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or extract both to a class nested in the helper?
like...
ScopeLimitationDetailBuilder.new(scope_limitation).call
This changes the proceeding_details partial IDs quite heavily to make testing easier.
Add the date to the end of the description if has not been added by the proceeding method. This is needed to remove the duplication of date values on the check your answer pages This may be a temp fix while scope limitation texts are reviewed
Rather than downcase each interpolated value, downcase the entire selector name
allow both upper and lower case emeregency or substantive types to be passed this allows a more uniform appearance in the the feature test scenarios
002d3e1
to
f43e70e
Compare
Quality Gate passedIssues Measures |
What
Link to story
This PR changes the proceeding_details partial IDs quite heavily to make testing easier.
Update the proceeding_helper to display the new logic required
Checklist
Before you ask people to review this PR:
bundle exec rake
git rebase main
.