Taxonomy email alert signup pages #33

malcolmbaig · 2017-03-23T11:00:00Z

Add the taxonomy email alert signup pages, served from /email-alert.

This PR includes some initial cleaning up of the test suite and dependencies - recommend using this diff to review the actual feature.

https://trello.com/c/lPlSbZnX/166-3-build-3-types-of-sign-up-pages-backend

New sign up (when child taxons present)

New sign up (when looking at leaf taxon)

Confirm page

- Remove unused @mock-email-alert-api tag. - Remove manually added lines from support/env.rb - this is a generated file and shouldn't be modified. - Rename the helper that adds GovukContentSchemaExamples to World, more accurately reflecting what it does. - Extract a slimmer helper from env.

Ensure that we can only stub methods that exist on instances of the GdsApi:EmailAlertApi object.

- Use the verifying instance double of email-alert-api in the existing email alert feature. - Setup a route specific to the test environment to test redirecting to govdelivery subscription url.

2.3.1 is the minimum version required by govuk_taxonomy_helpers.

Update frontend toolkit and add elements to bring in necessary sass for checkboxes.

Add show/hide content javascript and show and hide form submits.

Includes addition of 'Get email alerts' to breadcrumb and refactoring of the TaxonomySignupsController.

Stops sporadic failures caused by network requests to static.

dwhenry

Some of the pages don't have a blank line at the bottom of them which would be good to add from a consistency point of view if your keen. :)

dwhenry · 2017-03-27T13:38:22Z

app/assets/stylesheets/application.scss

@@ -15,3 +17,5 @@
    margin: 20px 0;
  }
 }
+
+@import "taxonomy-signup"


This is duplicated from line 5 above

dwhenry · 2017-03-27T13:48:06Z

app/controllers/taxonomy_signups_controller.rb

+  def confirm
+    redirect_to '/' and return unless params[:'taxon-list'].present?
+
+    taxon_path = params[:'taxon-list']


we look to have called the same thing 3 different names in the params: paths, taxon-list and taxon_path are we able to use something consistent or are these three different things?

Yep, naming consistency needs looking at - was going to do this in a followup PR but i'll commit something here.

Updated - I've opted to call the query param 'topic' across the controller and view. This is how we describe taxons to users so probably sensible to use that in the URL.

dwhenry · 2017-03-27T13:51:49Z

app/controllers/taxonomy_signups_controller.rb

+    redirect_to '/' and return unless params[:paths].present?
+
+    taxon_path = params[:paths].first
+    load_taxon(taxon_path)


what happens here if this fails to find the taxon? does the breadcrumb stuff breaks should it redirect?

Breaks noisily if it can't find a taxon with the given base path, which is reasonable i think.

@dwhenry added a controller test for this.

dwhenry · 2017-03-27T13:55:06Z

app/models/taxonomy_signup.rb

+    {
+      'title' => taxon['title'],
+      'links' => {
+        'taxons' => [taxon['content_id']]


we have called this taxon_tree when it is passed through with the content_item so I think that is the field we should be subscribing to.

quite right, updated.

dwhenry · 2017-03-27T14:02:17Z

app/views/taxonomy_signups/new.html.erb

+
+        <p class='email-option form-block'>or only one of the following</p>
+
+        <% @taxon.dig('links', 'child_taxons').each_with_index do |taxon, taxon_index| %>


might be nicer to use:

<%= render partial: "taxon", collection: @taxon.dig('links', 'child_taxons') %>

this would give youn the index variable in then view rather than having to pass it in.

not worth it in this case, less clear whats going on - particularly as you then need to handle a default value for when you're rendering the parent taxon with this partial.

dwhenry · 2017-03-27T14:22:12Z

app/controllers/taxonomy_signups_controller.rb

@@ -0,0 +1,43 @@
+class TaxonomySignupsController < ApplicationController
+  def new
+    redirect_to '/' and return unless params[:paths].present?


should we be redirecting back rather than to root?

That would break in the absence of a referer - redirecting to root is the simplest thing to do in this case, especially as you'd never have a link on the site without the paths specified.

dwhenry · 2017-03-27T14:23:12Z

app/views/layouts/application.html.erb

@@ -3,6 +3,7 @@
  <head>
    <title><%= yield :title %> - GOV.UK</title>
    <%= stylesheet_link_tag "application" %>
+    <%= javascript_include_tag 'application' %>


shouldn't this only be loaded in the footer? or at least lazy-loaded

I think that doesn't work in our layouts, this is the way it's done in the frontends I've seen.

moved, although as discussed this is handled by slimmer (checked with @NickColley )

tijmenb · 2017-03-27T14:54:04Z

Can you add some screenshots?

malcolmbaig · 2017-03-27T15:09:46Z

@tijmenb added

NickColley · 2017-03-27T15:35:54Z

Gemfile

@@ -3,7 +3,8 @@ source 'https://rubygems.org'
 gem 'rails', '4.2.5.2'
 gem 'slimmer', '10.0.0'

-gem 'govuk_frontend_toolkit', '~> 3.1.0'
+gem 'govuk_elements_rails', '~> 3.0.1'


We're currently not using GOV.UK Elements on any GOV.UK Publishing frontend applications.

Seems as though the kind of UI you're implementing it makes sense to make use of Elements, just Elements is technically built in a different way to the rest of GOV.UK Publishing frontend.

Maybe let's have a chat through it offline

As far as I know you don't have or hardly have anything like this on GOV.UK. Looks like you had a chat about it though, so that's good.

NickColley · 2017-03-27T15:39:54Z

app/assets/javascripts/show-hide-content.js

@@ -0,0 +1,172 @@
+;(function (global) {


What's the difference between this and https://github.com/alphagov/govuk_frontend_toolkit/blob/c88df4977fbb1d63950e8d19cc7dfbd02ede0709/javascripts/govuk/show-hide-content.js ?

@NickColley nothing, from the looks of it. The frontend work in this PR is almost entirely @carolinegreen . I would assume that with the removal of govuk_frontend_toolkit from this project, the show-hide-content javascript was simply copied across out of pragmatism.

@mobaig @carolinegreen did govuk_frontend_toolkit conflict with govuk_elements?

@NickColley Unfortunately @carolinegreen is on holiday this week : (.

No, I wasn't familiar with rails asset pipeline, and was making sure it worked before I put that in. Managed to miss this, I've put in a pull request for this now.

#34

NickColley · 2017-03-27T15:48:44Z

In your screenshots looks like there are indentation issues, the breadcrumbs should line up with the body of the page.

NickColley · 2017-03-28T08:52:05Z

app/views/taxonomy_signups/confirm.html.erb

+<%= render 'govuk_component/breadcrumbs', breadcrumbs: @breadcrumbs %>
+
+<div class='email-signup column-two-thirds'>
+<div class="govuk-title length-long">


This might work with http://govuk-component-guide.herokuapp.com/components/title

Updated to use the component instead.

Specifically, these failures scenarios: - the absence of the expected query param - not finding an item in the content store

- Add missing div and class for grid system, thus correcting alignment of page with breadcrumbs. - Use a govuk_component for the title on both signup pages.

dwhenry · 2017-03-28T11:25:55Z

As discussed would prefer not to use topic in the url, but understand the reasoning - nice external urls - so feel it is fine to do in this case.

malcolmbaig force-pushed the mb-taxonomy-email-signup branch from 397211a to df7e112 Compare March 24, 2017 17:00

Mo Baig and others added 12 commits March 25, 2017 10:47

Upgrade RSpec from 3.2 to 3.5

1f7225e

Mock out email-alert-api in features with a verifying double

674af22

Ensure that we can only stub methods that exist on instances of the GdsApi:EmailAlertApi object.

Use mock email-alert-api in email alert feature, also test redirects

37ecea4

- Use the verifying instance double of email-alert-api in the existing email alert feature. - Setup a route specific to the test environment to test redirecting to govdelivery subscription url.

Upgrade ruby version from 2.3.0 to 2.3.1

7e9934e

2.3.1 is the minimum version required by govuk_taxonomy_helpers.

Add email signup flow and associated cucumber feature

ddd8af9

Update frontend toolkit and add elements

0a13d1f

Update frontend toolkit and add elements to bring in necessary sass for checkboxes.

Add markup, copy tweaks and styling

c4eef7d

Add show/hide content

e678a52

Add show/hide content javascript and show and hide form submits.

Remaining frontend work to implement design from prototype

e5d9bad

Includes addition of 'Get email alerts' to breadcrumb and refactoring of the TaxonomySignupsController.

Amend taxon key used when creating subscription

5a36406

Add slimmer to rspec setup

ce64de0

Stops sporadic failures caused by network requests to static.

carolinegreen force-pushed the mb-taxonomy-email-signup branch from efa23f8 to 3259a2d Compare March 26, 2017 20:38

malcolmbaig force-pushed the mb-taxonomy-email-signup branch from 3259a2d to 272524f Compare March 26, 2017 21:16

carolinegreen force-pushed the mb-taxonomy-email-signup branch from 17f6d14 to 496395a Compare March 27, 2017 06:59

malcolmbaig force-pushed the mb-taxonomy-email-signup branch 2 times, most recently from 745c1c1 to d9c455c Compare March 27, 2017 12:49

malcolmbaig changed the title ~~[DO NOT MERGE] [WIP] taxonomy email signup~~ Taxonomy email alert signup Mar 27, 2017

malcolmbaig force-pushed the mb-taxonomy-email-signup branch 3 times, most recently from 09319a5 to b15d158 Compare March 27, 2017 13:05

malcolmbaig changed the title ~~Taxonomy email alert signup~~ Taxonomy email alert signup pages Mar 27, 2017

dwhenry suggested changes Mar 27, 2017

View reviewed changes

NickColley reviewed Mar 27, 2017

View reviewed changes

NickColley reviewed Mar 28, 2017

View reviewed changes

malcolmbaig force-pushed the mb-taxonomy-email-signup branch from 781ab90 to 09cf32e Compare March 28, 2017 10:36

Mo Baig added 3 commits March 28, 2017 12:05

Make naming of signup query param more consistent

1db8521

Add some additional testing of TaxonomySignupsController

a9b2a45

Specifically, these failures scenarios: - the absence of the expected query param - not finding an item in the content store

Frontend tweaks to taxonomy email signup pages

9d04d85

- Add missing div and class for grid system, thus correcting alignment of page with breadcrumbs. - Use a govuk_component for the title on both signup pages.

malcolmbaig force-pushed the mb-taxonomy-email-signup branch from 619e75b to b0cf74e Compare March 28, 2017 11:05

dwhenry approved these changes Mar 28, 2017

View reviewed changes

malcolmbaig force-pushed the mb-taxonomy-email-signup branch from 011aa7e to ce64de0 Compare March 28, 2017 11:32

malcolmbaig merged commit 50f9ccb into master Mar 28, 2017

malcolmbaig deleted the mb-taxonomy-email-signup branch March 28, 2017 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Taxonomy email alert signup pages #33

Taxonomy email alert signup pages #33

malcolmbaig commented Mar 23, 2017 •

edited

Loading

dwhenry left a comment

dwhenry Mar 27, 2017

malcolmbaig Mar 27, 2017

dwhenry Mar 27, 2017

malcolmbaig Mar 27, 2017

malcolmbaig Mar 28, 2017

dwhenry Mar 27, 2017

malcolmbaig Mar 27, 2017

malcolmbaig Mar 28, 2017

dwhenry Mar 27, 2017

malcolmbaig Mar 27, 2017

dwhenry Mar 27, 2017

malcolmbaig Mar 27, 2017

dwhenry Mar 27, 2017

malcolmbaig Mar 27, 2017

dwhenry Mar 27, 2017

tijmenb Mar 27, 2017

malcolmbaig Mar 27, 2017 •

edited

Loading

tijmenb commented Mar 27, 2017

malcolmbaig commented Mar 27, 2017

NickColley Mar 27, 2017

carolinegreen Mar 28, 2017

NickColley Mar 27, 2017

malcolmbaig Mar 28, 2017

NickColley Mar 28, 2017

malcolmbaig Mar 28, 2017

carolinegreen Mar 28, 2017

NickColley commented Mar 27, 2017

NickColley Mar 28, 2017

malcolmbaig Mar 28, 2017

dwhenry commented Mar 28, 2017


		<p class='email-option form-block'>or only one of the following</p>

		<% @taxon.dig('links', 'child_taxons').each_with_index do \|taxon, taxon_index\| %>

Taxonomy email alert signup pages #33

Taxonomy email alert signup pages #33

Conversation

malcolmbaig commented Mar 23, 2017 • edited Loading

New sign up (when child taxons present)

New sign up (when looking at leaf taxon)

Confirm page

dwhenry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malcolmbaig Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

tijmenb commented Mar 27, 2017

malcolmbaig commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickColley commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwhenry commented Mar 28, 2017

malcolmbaig commented Mar 23, 2017 •

edited

Loading

malcolmbaig Mar 27, 2017 •

edited

Loading