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

Remove time dependent deployment from transition landing page #1475

Merged
merged 10 commits into from Feb 3, 2020

Conversation

@huwd
Copy link
Contributor

huwd commented Feb 3, 2020

Trello: https://trello.com/c/vkNzXwcO/444-tidy-up-behind-ourselves-timed-deployment-edition

For the 31st of January (Brexit night) we had to deploy a version of the page that held both the before and after content, with a time based switch to flick between the two.

This allowed us to do a to-the-minute update.

With that moment past, we can now remove all that complexity!

Also

  1. Fixes one part where we were still reading from the old Brexit translations (the content was the same so we didn't spot it)
  2. Changes the mailto subject lines (for email + linked in) to exist in the translations file with everything else.

NB: We have no welsh translation for the subject line currently. Will request this, and add to a later PR. There is no change from current functionality on live.
Would be especially good if someone could check Linkedin works, as I don't have an account!

@bevanloon bevanloon temporarily deployed to govuk-collec-unwind-tim-cmfyzn Feb 3, 2020 Inactive
@bevanloon bevanloon temporarily deployed to govuk-collec-unwind-tim-cmfyzn Feb 3, 2020 Inactive
@bevanloon bevanloon temporarily deployed to govuk-collec-unwind-tim-cmfyzn Feb 3, 2020 Inactive
@huwd huwd requested review from vanitabarrett, koetsier and laurentqro Feb 3, 2020
@vanitabarrett

This comment has been minimized.

Copy link
Contributor

vanitabarrett commented Feb 3, 2020

Seems good, but looks like the build is failing due to tests

@huwd

This comment has been minimized.

Copy link
Contributor Author

huwd commented Feb 3, 2020

yeah sorry just spotted that, fixing now!

@bevanloon bevanloon temporarily deployed to govuk-collec-unwind-tim-cmfyzn Feb 3, 2020 Inactive
@bevanloon bevanloon temporarily deployed to govuk-collec-unwind-tim-cmfyzn Feb 3, 2020 Inactive
@huwd huwd marked this pull request as ready for review Feb 3, 2020
@huwd huwd changed the title [WIP] Remove time dependentdent deployment from transition landing page Remove time dependentdent deployment from transition landing page Feb 3, 2020
@bevanloon bevanloon temporarily deployed to govuk-collec-unwind-tim-cmfyzn Feb 3, 2020 Inactive
@vanitabarrett vanitabarrett changed the title Remove time dependentdent deployment from transition landing page Remove time dependent deployment from transition landing page Feb 3, 2020
@vanitabarrett

This comment has been minimized.

Copy link
Contributor

vanitabarrett commented Feb 3, 2020

If we're removing reference to the dynamic list, we should also remove it here: 8df4752#diff-605aa9fbcbf62bbb1853aa386ed7ad86R7

It's also scattered elsewhere in the code, probably worth removing as I think we've removed all the related view code that actually shows the chevron/link, so it doesn't look any different for true or false.

Previously these had passed because the time based switch showed the old content before 11pm 31st Jan. It's now after that so they fail. I've updated them to use the new content
@huwd huwd force-pushed the unwind-timed-deploy-madness branch from 2d8b6a9 to 4bc631d Feb 3, 2020
@bevanloon bevanloon temporarily deployed to govuk-collec-unwind-tim-cmfyzn Feb 3, 2020 Inactive
def fetch_buckets
buckets = I18n.t("#{time_based_intl}.campaign_buckets")
buckets = I18n.t("transition_landing_page.campaign_buckets")

This comment has been minimized.

Copy link
@laurentqro

laurentqro Feb 3, 2020

Contributor

somehow not in the scope of this PR, but should we have t instead of I18n.t for consistency?

This comment has been minimized.

Copy link
@laurentqro

laurentqro Feb 3, 2020

Contributor

or is t available only as a view helper?

This comment has been minimized.

Copy link
@huwd

huwd Feb 3, 2020

Author Contributor

Yeah, i checked this - so if you remove it here in the presenter it errors!!
However there appears to be some kind of override in helpers that must do something like:
t = I18n.t, which is a bit annyoing.

Screenshot_2020-02-03 Action Controller Exception caught

Would kind of like to throw it out to the community and ask what a good standard should be then write a rubocop rule for it?

t() is concise, but seems a bit orphan I18n.t() always seems clear what it is and where it comes from... so i might stay towards the more verbose?

This comment has been minimized.

Copy link
@laurentqro

laurentqro Feb 3, 2020

Contributor

Ok, so it looks like t is an ActionView helper, so we can't use t outside of views.

https://apidock.com/rails/v5.2.3/ActionView/Helpers/TranslationHelper/translate

@huwd huwd merged commit 17fac6d into master Feb 3, 2020
3 checks passed
3 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/publishing-e2e-tests Publishing end-to-end tests succeeded on Jenkins
Details
continuous-integration/jenkins/security No security issues found
Details
@huwd huwd deleted the unwind-timed-deploy-madness branch Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.