MISUV-9024 - Reporting frequency page entry point #2728
Conversation
TomRafferty
force-pushed
the
MISUV-9024
branch
2 times, most recently
from
a188c06 to
2d3edaf
Compare
TomRafferty
force-pushed
the
MISUV-9024
branch
from
2d3edaf to
846e14d
Compare
| Html( | ||
| s""" | ||
| <span id="one-year-opt-out-message"> | ||
| ${messages("nextUpdates.optOutOneYear-1", m.startYear, m.endYear)} | ||
| ${link(id = Some("single-year-opt-out-warning-link"), link = controllers.optOut.routes.SingleYearOptOutWarningController.show(isAgent).url, messageKey = "nextUpdates.optOutOneYear-2")} | ||
| ${messages("nextUpdates.withReportingFrequencyContent.optOutOneYear-1", m.startYear, m.endYear)} |
There was a problem hiding this comment.
think we can simplify this a bit and move some logic out of the views.
We can we use pattern match guards to determine the most of the html and link redirects etc.
Then just pass like a model of everything into a single.
Likely can move it up once and put the logic in the controller or maybe a view model/helper of sorts.
Html(
s"""
<span id="one-year-opt-out-message">
${messages("nextUpdates.optOutOneYear-1", m.startYear, m.endYear)}
${link(id = Some("single-year-opt-out-warning-link"), link = controllers.optOut.routes.SingleYearOptOutWarningController.show(isAgent).url, messageKey = "nextUpdates.optOutOneYear-2")}
</span>
"""
)
There was a problem hiding this comment.
formatting is a bit of a pain in github comments sorry.
| @@ -59,7 +59,7 @@ class NextUpdatesController @Inject()(NoNextUpdatesView: NoNextUpdates, | |||
| } | |||
| } | |||
|
|
|||
| def getNextUpdates(backUrl: Call, isAgent: Boolean, errorHandler: ShowInternalServerError, origin: Option[String] = None) | |||
| def getNextUpdates(backUrl: Call, isAgent: Boolean, errorHandler: ShowInternalServerError, origin: Option[String] = None, showReportingFrequencyContent: Boolean = false) | |||
There was a problem hiding this comment.
think we don't need a boolean flag here as a parameter and as a suggestion can just call the feature switch inside this method?
val shouldShowReportingContent: Boolean = isEnabled(ReportingFrequencyPage)
also I try to not set default values for params unless it's for a good reason or massive syntax convenience. Helps with spotting issues sooner with the code. Since the value is not defaulted, if the value needs to be passed around a lot it explicitly defined at each point.
There was a problem hiding this comment.
will look into all the comments ta 😄
There was a problem hiding this comment.
wonder what you think about something like this?
| @@ -127,6 +156,7 @@ <h2 class="govuk-heading-m">@messages("nextUpdates.sub-heading")</h2> | |||
| } | |||
| } | |||
|
|
|||
| @import models.admin.ReportingFrequencyPage | |||
There was a problem hiding this comment.
this import is not needed
| nextUpdates.optOutOneYear-1 = You are currently reporting quarterly on a voluntary basis for the {0} to {1} tax year. You can choose to | ||
| nextUpdates.optOutOneYear-2 = opt out of quarterly updates and report annually instead. | ||
| nextUpdates.withReportingFrequencyContent.optOutOneYear-1 = Depending on your circumstances, you may be able to |
There was a problem hiding this comment.
I guess with these keys -1 and -2 can they be a little bit more descriptive
maybe instead of -1 perhaps .p.message and -2 could be .p.link
| nextUpdates.withReportingFrequencyContent.optOutOneYear-2 = view and change your reporting frequency. | ||
|
|
||
| nextUpdates.optOutMultiYear-1 = You are currently reporting quarterly on a voluntary basis. You can choose to | ||
| nextUpdates.optOutMultiYear-2 = opt out of quarterly updates and report annually instead. | ||
| nextUpdates.withReportingFrequencyContent.optOutMultiYear-1 = Depending on your circumstances, you may be able to | ||
| nextUpdates.withReportingFrequencyContent.optOutMultiYear-2 = view and change your reporting frequency. |
There was a problem hiding this comment.
Currently i'm doing the ATs for this and I noticed that the added messages (Specifically "view and change your reporting frequency") aren't being used within the views you edited
There was a problem hiding this comment.
Also can you do me a favour and add IDs to the opt in/out links in the reporting frequency page (https://github.com/hmrc/income-tax-view-change-frontend/blob/main/app/views/ReportingFrequencyView.scala.html#L61-L79) so i can get selenium to click them
There was a problem hiding this comment.
Hi Milo, will reach out on slack - Michael has been working on an alternate implementation of this and we've been working on another branch 😄
closing this PR now and will open one from that branch, thanks
No description provided.