Add form for a person to contribute suggested councillor info #1208

hisayohorie · 2017-08-01T12:56:56Z

This is related to this issue openaustralia/australian_local_councillors_popolo#98 (comment)

This PR is the most basic first version of how people contribute councillor info, so that we can load the information to the site.
People can go to this page from each authorities, and put the names and emails of the councillors they want to contribute. After they submit their contribution, optionally they can give us their contact information(name and email), so that we can contact them to thank them.

The suggested councillors appear on admin page, and admin can copy them in the existing process of adding new councillors.

When this is deployed, migration needs to run to add new database tables.

…ustralia/planningalerts into person_contributes_councillor_info

… test

…gested_councillors controllor

…ncillo in authority model, and changed the create method to use build in suggested_councillor controllerr

…ontributes_councillor_info_2 Added model, controller and view for suggested councillors.

Start of openaustralia#1176

…flag

… page

The test for Council Name is nested in the feature_flag_is_on test, so that the feature is visible in the test environment.

Feature flag

to make it more readable and clear. still need some styling.

To separate the test exercise and test expectation.

…cillor_council_name Suggested councillor council name

to add a contributor info form in the suggested councillor.

The user has just submitted all these councillors themselves, so I don't think they need to be told where this list came from. This heading is also confusing as it comes above the form to put in a new councillor.

The heading of the page explains that this is for adding councillors, so I don't think we need to restate that here. This commit also changes the tests to use css to target the fieldset, rather than the text of the legend (now missing).

This makes it more the focus of the page and sets it apart from the other details, like the labels.

This pushes the actual data you're looking at forward a bit.

There's a bug in there flexbox implementation that a fieldset element can't be a flex container. Adding a wrapper div is the workaround. See https://github.com/philipwalton/flexbugs#9-some-html-elements-cant-be-flex-containers

…ontribution_basic_styles Add basic styles to the councillor contribution form

equivalentideas

@hisayohorie and I went through this and we found a few bits we want to improve before merging.

Looking really good though 💯 😄 looking forward to getting this deployed!

equivalentideas · 2017-08-01T13:00:16Z

app/controllers/contributors_controller.rb

+    end
+  end
+
+private


This should be indented to the same level as the methods it covers below.

equivalentideas · 2017-08-01T13:07:37Z

app/models/contributor.rb

@@ -0,0 +1,4 @@
+class Contributor < ActiveRecord::Base
+  has_many :suggested_councillors, through: :councillor_contributions


Is this association necessary? If we're not using it we should remove that line.

suggested_councillor_spec.rbanduser_contributes_new_councillor_spec.rbpass without this association. Currently we don't have the controller/councillor_contribution_controller_spec.rbnor model/contributor_spec_rbso if we get to the stage to have the test, we may need the association? but for now I deleted it.

equivalentideas · 2017-08-01T13:07:42Z

app/models/authority.rb

@@ -21,6 +21,9 @@ class Authority < ActiveRecord::Base
  has_many :applications
  has_many :councillors
  has_many :comments, through: :applications
+  has_many :councillor_contributions
+  has_many :suggested_councillors, through: :councillor_contributions


Is this association necessary? If we're not using it we should remove that line.

test will pass without it, but the actual code won't run. It returns error because councillor_contribution#new needs authority_id. So I'm keeping this association. Also we have to create controller/councillr_contributions_controller_spec.rb.

equivalentideas · 2017-08-01T13:11:54Z

app/views/contributors/new.html.haml

@@ -0,0 +1,20 @@
+%h1 Thank you!
+%p
+  This data you contributed will be reviewd


Should be "reviewed".

equivalentideas · 2017-08-01T13:12:45Z

app/views/contributors/new.html.haml

+%h1 Thank you!
+%p
+  This data you contributed will be reviewd
+  by an administrator, and go live shortly.


I think we can improve this text a bit but that's not a problem for this first pass being merged. We should open an issue for that.

equivalentideas · 2017-08-01T13:23:38Z

spec/features/user_contributes_new_councillor_spec.rb

+        fill_in "Email", with: "mgilic@casey.vic.gov.au"
+      end
+
+      click_button "Submit"


I think this should use the full button text like the other tests do for consistency.

We decided to change this so that all the times this button is hit, the full text is used "Submit 1 new councillor".

equivalentideas · 2017-08-01T13:27:28Z

spec/features/user_contributes_new_councillor_spec.rb

+        fill_in "Email", with: "mgilic@casey.vic.gov.au"
+      end
+
+      click_button "Add another councillor"


@hisayohorie and I talked about this, and we recon that this test could be more focused if the person only adds one councillor. That keeps the focus on them not adding there contributor information.

I came to realize that this test is to check it works successfully when the contributor does not provide their information, and the test is not what it's described. I changed the test to check what it described. I will make a change in the view page to pass the test in separate PR

@hisayohorie and I decided to try and make these tests more focused by splitting the options for the contributor to tell us their contact information into a new feature spec file 74466ea

equivalentideas · 2017-08-01T13:28:12Z

spec/features/user_contributes_new_councillor_spec.rb

+      expect(page).to have_content "Thank you"
+    end
+
+    it "works successfully without contributor information" do


To be consistent with the test above, I recon the description here should be something like "works successfully when the contributor does not provide their information"

equivalentideas · 2017-08-01T13:29:00Z

spec/features/user_contributes_new_councillor_spec.rb

+      click_button "Submit 3 new councillors"
+
+      expect(page).to have_content "Thank you"
+


Stray empty line.

equivalentideas · 2017-08-01T13:30:14Z

spec/models/contributor_spec.rb

+require 'spec_helper'
+
+RSpec.describe Contributor, type: :model do
+  pending "add some examples to (or delete) #{__FILE__}"


Let's remove this file if we're not adding tests yet.

For clarification.

For consistency.

For clarification.

This responds to a comment here: openaustralia#1208 (comment)

This makes it clearer to the test reader which button is being pressed here, and it also adds slightly to the test to make sure that button text is what we intend. Relates to discussion: openaustralia#1208 (comment)

For proper gramma.

To give thank you for contributor who did not leave their info.

This creates a new file that covers the different options for contribution for providing their information to us. We split this out to make the original spec file more focused and easier to understand. This now means there isn't a single spec for working through the whole contribution process from beginning to end. But we think that's ok for now, because the contribution and the stage of providing details are quite separate.

equivalentideas

@hisayohorie nice work making all these fixes :) I think we made the tests clearer in particular :)

I think this is ready to go. I'll merge it and deploy! 🚀

equivalentideas · 2017-08-02T03:53:03Z

config/routes.rb

+  post "/authorities/:authority_id/councillor_contributions/new", to: "councillor_contributions#new"
+
+  resources :contributors, only:[:new, :create]
+    get "/contributors/no_info", to: "contributors#no_contributor_info"


I think this should change to something a little more specific like "contributors/anonymous", but we don't need to do that before deploying.

equivalentideas · 2017-08-02T04:23:25Z

README.md

+The feature is globally enabled when the value of `ENV["CONTRIBUTE_COUNCILLORS_ENABLED"]` is `"true"`.
+This flag is useful if you need to turn the feature _off_ globally.
+
+We set this in the [`.env`](https://github.com/openaustralia/planningalerts/blob/master/.env) file in production.  You can control setting in development by creating your own `.env.development` file which includes:


Actually we set it in production in .env.production. We should update that.

equivalentideas and others added 30 commits June 15, 2017 15:52

Add failing test for contributing a new councillor

8fab20b

Add failing spec for admin viewing suggested councillor

0afe7ed

Merge branch 'person_contributes_councillor_info' of github.com:opena…

d46eb58

…ustralia/planningalerts into person_contributes_councillor_info

added route for suggested_councillors

aa608fb

generated suggested_councillor model and sugested_councillors controller

edd6fb5

created controller, model, and view for suggested councillors. passes…

ce701b8

… test

fixed the styling, removed irrelevant files and wrote a test for sug…

5523f66

…gested_councillors controllor

changed the syntax to Ruby 1.9

969ef04

fixed an indentation, changed the test, added has_many :suggested_cou…

5ead1a5

…ncillo in authority model, and changed the create method to use build in suggested_councillor controllerr

fixed the typos in the suggested_councilor_spec.rb

6bcaf48

deleted an extra line in suggested_councillors_spec.rb

18e56ea

Fixed a typo on suggested_councillor_spec.rb

330d891

Merge pull request openaustralia#1174 from hisayohorie/hisayo_prson_c…

a5c81bc

…ontributes_councillor_info_2 Added model, controller and view for suggested councillors.

Merge branch 'master' into person_contributes_councillor_info

ea92d1a

added a suggested_councillor in factories.rb

a3b2e5a

generated admin/suggested_councillors.rb

dd133ac

Use authority shortname in routes for consistency

9a277e7

If the feature flag is off, return a 404 page

8bfa815

Start of openaustralia#1176

Added before_action:check_if_feature_flag_is_on to to enable feature …

610d10c

…flag

added an extra line for style

f157f08

added a text to check council's name is displayed on the contribution…

52d5a20

… page

Adding the Council Name in the view suggested_councillor#new

d4b84ae

Correctly nest a test in feature_flag is on

119d9e9

The test for Council Name is nested in the feature_flag_is_on test, so that the feature is visible in the test environment.

Added an extra line for styling

21b3e57

Merge pull request openaustralia#1184 from hisayohorie/feature_flag

bb91a6c

Feature flag

Added a header in suggested councillor contribution page

fd92a83

to make it more readable and clear. still need some styling.

Added an extra line for styling

56cfaec

To separate the test exercise and test expectation.

Merge pull request openaustralia#1185 from hisayohorie/suggested_coun…

6c15cb0

…cillor_council_name Suggested councillor council name

Add failing test for collecting contributor information

9295e5b

Created contributor_model

e31c3dc

to add a contributor info form in the suggested councillor.

equivalentideas and others added 9 commits August 1, 2017 12:39

Remove unnecessary heading

3e7e125

The user has just submitted all these councillors themselves, so I don't think they need to be told where this list came from. This heading is also confusing as it comes above the form to put in a new councillor.

Remove legend with redundant label

ed9c112

The heading of the page explains that this is for adding councillors, so I don't think we need to restate that here. This commit also changes the tests to use css to target the fieldset, rather than the text of the legend (now missing).

Make the councillor data text bigger

54d8257

This makes it more the focus of the page and sets it apart from the other details, like the labels.

Fade the labels of submitted councillors

0c9d3af

This pushes the actual data you're looking at forward a bit.

Remove unused style

ec8982f

Fix layout bug for Safari and Chrome

d921a16

There's a bug in there flexbox implementation that a fieldset element can't be a flex container. Adding a wrapper div is the workaround. See https://github.com/philipwalton/flexbugs#9-some-html-elements-cant-be-flex-containers

Reuse class and remove redundant style declaration (refactor only)

31c521c

Merge pull request openaustralia#1207 from openaustralia/councillor_c…

66ef9e2

…ontribution_basic_styles Add basic styles to the councillor contribution form

Fixed the description in test

574707a

equivalentideas suggested changes Aug 1, 2017

View reviewed changes

hisayohorie added 10 commits August 1, 2017 09:36

Fixed indentation

442a3b3

Deleted unused association

cb1701d

Fixed a typo

667d55e

Updated the test description

2b293f9

For clarification.

Integrated overlapping specs

9772a59

Fixed the submit button text

3d4e7da

For consistency.

Fixed the description of a spec

3a1d23c

For clarification.

Fixed the test to reflect the description accurately

0541b2c

Removed extra line

7a7eb4c

Deleted unused spec file

263473f

hisayohorie mentioned this pull request Aug 1, 2017

Fixing style and unused association to merge person_contributes_councillor_info branch #1209

Closed

equivalentideas and others added 6 commits August 2, 2017 12:56

Remove unnecessary association

e091e13

This responds to a comment here: openaustralia#1208 (comment)

Use full button text in test

c24a8bb

This makes it clearer to the test reader which button is being pressed here, and it also adds slightly to the test to make sure that button text is what we intend. Relates to discussion: openaustralia#1208 (comment)

Pluralize "councillor" on the button

9c4783c

For proper gramma.

Remove stray empty line (whitespace only)

b1438a8

Added no_contributor_info route to show flash notice thank you

8a04ea2

To give thank you for contributor who did not leave their info.

equivalentideas approved these changes Aug 2, 2017

View reviewed changes

equivalentideas merged commit 2f9e80c into openaustralia:master Aug 2, 2017

equivalentideas reviewed Aug 2, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add form for a person to contribute suggested councillor info #1208

Add form for a person to contribute suggested councillor info #1208

hisayohorie commented Aug 1, 2017

equivalentideas left a comment

equivalentideas Aug 1, 2017

equivalentideas Aug 1, 2017

hisayohorie Aug 1, 2017 •

edited

equivalentideas Aug 1, 2017

hisayohorie Aug 1, 2017

equivalentideas Aug 1, 2017

hisayohorie Aug 1, 2017

equivalentideas Aug 1, 2017

equivalentideas Aug 1, 2017

hisayohorie Aug 1, 2017

equivalentideas Aug 2, 2017

equivalentideas Aug 1, 2017

hisayohorie Aug 1, 2017

equivalentideas Aug 2, 2017

equivalentideas Aug 1, 2017

hisayohorie Aug 1, 2017

equivalentideas Aug 1, 2017

hisayohorie Aug 1, 2017

equivalentideas Aug 1, 2017

hisayohorie Aug 1, 2017

equivalentideas left a comment

equivalentideas Aug 2, 2017

equivalentideas Aug 2, 2017

+                  end
+                end
+              private

		@@ -0,0 +1,4 @@
		class Contributor < ActiveRecord::Base
		has_many :suggested_councillors, through: :councillor_contributions

		click_button "Submit 3 new councillors"

		expect(page).to have_content "Thank you"

Add form for a person to contribute suggested councillor info #1208

Add form for a person to contribute suggested councillor info #1208

Conversation

hisayohorie commented Aug 1, 2017

equivalentideas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hisayohorie Aug 1, 2017 • edited

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

Choose a reason for hiding this comment

equivalentideas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hisayohorie Aug 1, 2017 •

edited