UI for setting the reason why SHF is waiting on an applicant #281

weedySeaDragon · 2017-06-21T16:41:44Z

PT Story: https://www.pivotaltracker.com/story/show/143810729

This is the 2nd PR (the 2nd half) for the story.

Changes proposed in this pull request:

Seed the db with 1 reason: the "other/custom" reason because it is used specifically. (Other reasons can be entered via the usual web pages/interface).
UI: the reasons for waiting has been moved to above the _application status buttons
Uses AJAX to update the db immediately when a reason is selected from the list. The db is also updated immediately when the text for a 'custom/other' reason is entered.

Screenshots (Optional):

The reason isn't shown if the status is not 'Waiting for Applicant'

The text field for typing in the "other reason" only shows if that reason is selected

Ready for review:
@patmbolger @thesuss @RobertCram

# Conflicts: # .ruby-version

…on_text; migrate db

… I18n locales)

…uttons/sections

…red out

# Conflicts: # db/seeds.rb

…n locale)

… changed (not just when page loaded)

houndci-bot · 2017-06-21T16:41:58Z

app/assets/stylesheets/member-app-waiting-reason.scss

+  }
+
+  input[type="text"]{
+    padding: 0.3rem 0.3rem;


Shorthand form for property padding should be written more concisely as 0.3rem instead of 0.3rem 0.3rem

houndci-bot · 2017-06-21T16:41:58Z

app/assets/stylesheets/member-app-waiting-reason.scss

+    height: 110%;
+  }
+
+  input[type="text"]{


Avoid qualifying attribute selectors with an element.
Opening curly brace { should be preceded by one space

houndci-bot · 2017-06-21T16:41:58Z

app/assets/stylesheets/member-app-waiting-reason.scss

@@ -1,7 +1,12 @@
-.member_app_waiting_reasons {
+#member_app_waiting_reasons {


Avoid using id selectors
Selector member_app_waiting_reasons should be written in lowercase with hyphens

houndci-bot · 2017-06-21T16:41:58Z

app/views/membership_applications/_reason_waiting.html.haml

@@ -0,0 +1,78 @@
+-# submit the changed reason via xhr (javascript) whenever the change from the )
+-#  Do not make user press a "save" or "submit" button for just these changes.


Comment should have a space after the #

houndci-bot · 2017-06-21T16:41:58Z

app/views/membership_applications/_reason_waiting.html.haml

@@ -0,0 +1,78 @@
+-# submit the changed reason via xhr (javascript) whenever the change from the )


2 consecutive comments can be merged into one

houndci-bot · 2017-06-21T16:41:59Z

app/views/membership_applications/_application_status_form.html.haml


-  = button_to t('membership_applications.reject_btn'), reject_membership_application_path(@membership_application), {class: 'btn reject', disabled: !(@membership_application.may_reject?)}
+    = button_to t('membership_applications.ask_applicant_for_info_btn'), need_info_membership_application_path(@membership_application), {class: 'btn need-info', disabled: !(@membership_application.may_ask_applicant_for_info?)}


Avoid using instance variables in partials views

houndci-bot · 2017-06-21T16:41:59Z

app/views/membership_applications/_application_status_form.html.haml


-  = button_to t('membership_applications.accept_btn'), accept_membership_application_path(@membership_application), {class: 'btn accept', disabled: !(@membership_application.may_accept?)}
+    = button_to t('membership_applications.reject_btn'), reject_membership_application_path(@membership_application), {class: 'btn reject', disabled: !(@membership_application.may_reject?)}


Avoid using instance variables in partials views

houndci-bot · 2017-06-21T16:41:59Z

app/views/membership_applications/_application_status_form.html.haml

+  .row
+    = button_to t('membership_applications.start_review_btn'), start_review_membership_application_path(@membership_application), {class: 'btn start-review', disabled: !(@membership_application.may_start_review?)}
+
+    = button_to t('membership_applications.accept_btn'), accept_membership_application_path(@membership_application), {class: 'btn accept', disabled: !(@membership_application.may_accept?)}


Avoid using instance variables in partials views

houndci-bot · 2017-06-21T16:41:59Z

app/views/membership_applications/_application_status_form.html.haml

 #admin-change-status-buttons
  %p.header
    = "#{t('membership_applications.show.change_status')}:"


-  = button_to t('membership_applications.start_review_btn'), start_review_membership_application_path(@membership_application), {class: 'btn start-review', disabled: !(@membership_application.may_start_review?)}
+  .row
+    = button_to t('membership_applications.start_review_btn'), start_review_membership_application_path(@membership_application), {class: 'btn start-review', disabled: !(@membership_application.may_start_review?)}


Avoid using instance variables in partials views

houndci-bot · 2017-06-21T16:41:59Z

app/helpers/membership_applications_helper.rb

+
+  def reason_method(method_prefix, locale)
+    possible_method = "#{method_prefix}_#{locale}".to_sym
+    method_name = (AdminOnly::MemberAppWaitingReason.new.respond_to?(possible_method) ? possible_method : AdminOnly::MemberAppWaitingReason.send("default_#{method_prefix}_method".to_sym))


Useless assignment to variable - method_name.

RobertCram · 2017-06-22T11:22:22Z

app/models/admin_only/member_app_waiting_reason.rb

-    def self.other_reason_name
-      I18n.t('admin_only.member_app_waiting_reasons.other_custom_reason')
+    def self.other_reason_name(locale = I18n.locale)
+      I18n.t('admin_only.member_app_waiting_reasons.other_custom_reason', locale)


The second parameter to the translate method should be an options hash. As it stands now the locale is ignored, and the method always returns the swedish translation

I18n.t('admin_only.member_app_waiting_reasons.other_custom_reason', locale: locale)

RobertCram · 2017-06-22T11:22:43Z

app/models/admin_only/member_app_waiting_reason.rb

    end


+    def self.other_reason_desc(locale = I18n.locale)
+      I18n.t('admin_only.member_app_waiting_reasons.other_custom_reason_desc', locale)
+    end


Same as above.

RobertCram · 2017-06-22T12:32:03Z

Great work, Ashley. Apart from my inline comment, two minor remarks:

There should probably be some sort of protection against changing or deleting the "other/custom reason" record?
In the UI, I would personally prefer the "Other (enter the reason)" to be the last option in the list

…del (not in the db)

…." reason option

…/tests

houndci-bot · 2017-06-23T22:10:09Z

features/step_definitions/assertion_steps.rb

+  select_options= case field.tag_name
+                  when 'select'
+                    options = field.all('option')
+                end


end at 492, 16 is not aligned with case at 489, 18.

houndci-bot · 2017-06-23T22:10:09Z

features/step_definitions/assertion_steps.rb

+
+  select_options= case field.tag_name
+                  when 'select'
+                    options = field.all('option')


Useless assignment to variable - options.

houndci-bot · 2017-06-23T22:10:09Z

app/assets/stylesheets/member-app-waiting-reason.scss

+    font-weight: normal;
+  }
+
+  input[type="text"] {


Avoid qualifying attribute selectors with an element.

houndci-bot · 2017-06-23T22:10:09Z

app/assets/stylesheets/member-app-waiting-reason.scss

+    padding-bottom: 0.5rem;
+  }
+
+  border-bottom: 1pt solid coral;


Expected item on line 11 to appear before line 4. Rule sets should be ordered as follows: @extends, @includes without @content, properties, @includes with @content, nested rule sets
Color coral should be written in hexadecimal form as #ff7f50
Color literals like coral should only be used in variable declarations; they should be referred to via variable everywhere else.

houndci-bot · 2017-06-23T22:10:09Z

app/assets/stylesheets/member-app-waiting-reason.scss

-    width: 3rem;
+  margin-bottom: 1rem;
+  .row {
+    margin-top: 0;


Properties should be ordered margin-bottom, margin-top, padding-bottom, padding-top

houndci-bot · 2017-06-23T22:10:09Z

app/assets/stylesheets/member-app-waiting-reason.scss

-  input.wpcf7-form-control.is-custom {
-    width: 3rem;
+  margin-bottom: 1rem;
+  .row {


Rule declaration should be preceded by an empty line

houndci-bot · 2017-06-23T22:10:09Z

app/assets/stylesheets/member-app-waiting-reason.scss


-  input.wpcf7-form-control.is-custom {
-    width: 3rem;
+  margin-bottom: 1rem;


Properties should be ordered border-bottom, margin-bottom

houndci-bot · 2017-06-23T22:10:09Z

features/step_definitions/assertion_steps.rb

+  select_options= case field.tag_name
+                  when 'select'
+                    options = field.all('option')
+                end


end at 492, 16 is not aligned with case at 489, 18.

houndci-bot · 2017-06-23T22:10:09Z

features/step_definitions/assertion_steps.rb

+
+  select_options= case field.tag_name
+                  when 'select'
+                    options = field.all('option')


Useless assignment to variable - options.

houndci-bot · 2017-06-23T22:10:10Z

app/assets/stylesheets/member-app-waiting-reason.scss

+    font-weight: normal;
+  }
+
+  input[type="text"] {


Avoid qualifying attribute selectors with an element.

houndci-bot · 2017-06-23T22:10:10Z

app/assets/stylesheets/member-app-waiting-reason.scss

+    padding-bottom: 0.5rem;
+  }
+
+  border-bottom: 1pt solid coral;


Expected item on line 11 to appear before line 4. Rule sets should be ordered as follows: @extends, @includes without @content, properties, @includes with @content, nested rule sets
Color coral should be written in hexadecimal form as #ff7f50
Color literals like coral should only be used in variable declarations; they should be referred to via variable everywhere else.

houndci-bot · 2017-06-23T22:10:10Z

app/assets/stylesheets/member-app-waiting-reason.scss

-    width: 3rem;
+  margin-bottom: 1rem;
+  .row {
+    margin-top: 0;


Properties should be ordered margin-bottom, margin-top, padding-bottom, padding-top

houndci-bot · 2017-06-23T22:10:10Z

app/assets/stylesheets/member-app-waiting-reason.scss

-  input.wpcf7-form-control.is-custom {
-    width: 3rem;
+  margin-bottom: 1rem;
+  .row {


Rule declaration should be preceded by an empty line

houndci-bot · 2017-06-23T22:10:10Z

app/assets/stylesheets/member-app-waiting-reason.scss


-  input.wpcf7-form-control.is-custom {
-    width: 3rem;
+  margin-bottom: 1rem;


Properties should be ordered border-bottom, margin-bottom

weedySeaDragon · 2017-06-23T22:16:48Z

I know the build fails. I can't figure out why the cucumber feature tests are failing. Any help is appreciated!

fyi: the feature that fail is this file: features/membership-application-status/admin-custom-reason-waiting.feature

(cherry picked from commit 75f5e10)

(cherry picked from commit 08ae9ba)

(cherry picked from commit bc1d1be)

(cherry picked from commit aa7f58b)

(cherry picked from commit 2bf19ed)

(cherry picked from commit 560b1ca)

houndci-bot · 2017-06-26T21:25:26Z

app/views/membership_applications/_reason_waiting.html.haml

+  .container
+    .reason-waiting-information
+      = label_tag :member_app_waiting_reasons, t('membership_applications.need_info.reason_title')
+      - collection = AdminOnly::MemberAppWaitingReason.all.to_a


3 consecutive Ruby scripts can be merged into a single :ruby filter

houndci-bot · 2017-06-26T21:25:27Z

app/views/membership_applications/_reason_waiting.html.haml

+  .container
+    #other-text-field
+      = label_tag :custom_reason_text, t('membership_applications.need_info.other_reason_label')
+      = text_field_tag :custom_reason_text, @membership_application.custom_reason_text


Avoid using instance variables in partials views

houndci-bot · 2017-06-26T21:25:27Z

app/views/membership_applications/_reason_waiting.html.haml

+      = label_tag :member_app_waiting_reasons, t('membership_applications.need_info.reason_title')
+      - collection = AdminOnly::MemberAppWaitingReason.all.to_a
+      - collection << AdminOnly::MemberAppWaitingReason.new(id: -1, "name_#{I18n.locale.to_s}": "#{@other_waiting_reason_text}")
+      - selected = ! @membership_application.custom_reason_text.blank? ? -1 : @membership_application.member_app_waiting_reasons_id


Avoid using instance variables in partials views

houndci-bot · 2017-06-26T21:25:27Z

app/views/membership_applications/_reason_waiting.html.haml

+    .reason-waiting-information
+      = label_tag :member_app_waiting_reasons, t('membership_applications.need_info.reason_title')
+      - collection = AdminOnly::MemberAppWaitingReason.all.to_a
+      - collection << AdminOnly::MemberAppWaitingReason.new(id: -1, "name_#{I18n.locale.to_s}": "#{@other_waiting_reason_text}")


Avoid using instance variables in partials views

houndci-bot · 2017-06-26T21:25:27Z

app/assets/stylesheets/member-app-waiting-reason.scss

+$reason-border-color:  #ff7f50;  // coral; picked sort of at random
+
+
+#member-app-waiting-reasons {


Avoid using id selectors

houndci-bot · 2017-06-26T21:25:27Z

app/views/membership_applications/_reason_waiting.html.haml

+  .container
+    .reason-waiting-information
+      = label_tag :member_app_waiting_reasons, t('membership_applications.need_info.reason_title')
+      - collection = AdminOnly::MemberAppWaitingReason.all.to_a


3 consecutive Ruby scripts can be merged into a single :ruby filter

houndci-bot · 2017-06-26T21:25:27Z

app/views/membership_applications/_reason_waiting.html.haml

+  .container
+    #other-text-field
+      = label_tag :custom_reason_text, t('membership_applications.need_info.other_reason_label')
+      = text_field_tag :custom_reason_text, @membership_application.custom_reason_text


Avoid using instance variables in partials views

houndci-bot · 2017-06-26T21:25:27Z

app/views/membership_applications/_reason_waiting.html.haml

+      = label_tag :member_app_waiting_reasons, t('membership_applications.need_info.reason_title')
+      - collection = AdminOnly::MemberAppWaitingReason.all.to_a
+      - collection << AdminOnly::MemberAppWaitingReason.new(id: -1, "name_#{I18n.locale.to_s}": "#{@other_waiting_reason_text}")
+      - selected = ! @membership_application.custom_reason_text.blank? ? -1 : @membership_application.member_app_waiting_reasons_id


Avoid using instance variables in partials views

houndci-bot · 2017-06-26T21:25:27Z

app/views/membership_applications/_reason_waiting.html.haml

+    .reason-waiting-information
+      = label_tag :member_app_waiting_reasons, t('membership_applications.need_info.reason_title')
+      - collection = AdminOnly::MemberAppWaitingReason.all.to_a
+      - collection << AdminOnly::MemberAppWaitingReason.new(id: -1, "name_#{I18n.locale.to_s}": "#{@other_waiting_reason_text}")


Avoid using instance variables in partials views

houndci-bot · 2017-06-26T21:25:27Z

app/assets/stylesheets/member-app-waiting-reason.scss

+$reason-border-color:  #ff7f50;  // coral; picked sort of at random
+
+
+#member-app-waiting-reasons {


Avoid using id selectors

weedySeaDragon · 2017-06-26T21:30:49Z

passing locally on my machine with selenium, but not here with semaphore.

teampoltergeist/poltergeist#895 Unfortunately parse time errors for ES6 features like *let* can be silently swallowed by PhantomJS, so you end up with no errors but also with no JS being actually run.

weedySeaDragon · 2017-06-27T20:10:20Z

Closing this. Will replace it with with a much cleaner approach that @patmbolger came up with. 👍

We sure did learn a lot about poltergeist, phantom, selenium, and data-remote (oh my!).
Hopefully we'll write up / document that. Meanwhile, where's a short list:

Lessons learned:

phantom.js will silently fail if you use let in your Javascript (ECMS 6.0). (Use 'var' instead) Ref: poltergeist issue 823: use of JavaScript's let will fail silently
waiting for a "Then I should not see" used expect(page).not_to have_content content. However, this does not make Capybara wait for ajax, which leads to all kind of messes (rails still running while the test is finished, etc.). (This made some of the tests fail due to timeout because they were waiting forever.)
use Capybara's assert_text instead
use data-remote to send info back to the controller, even if the parameters aren't what is 'normally' expected by the Controller. Let the controller parse and deal with whatever parameters are sent. Dealing with them in your Javascript is ugly and harder to maintain.

patmbolger · 2017-06-27T21:33:16Z

Good list!

In the 2nd item, the assert_ Capybara methods do not wait on outstanding AJAX requests to complete, but they do wait as long as the Capybara timeout setting (which I think is 10 seconds by default). This is usually a much better approach than using Rspec expect.

A 4th item is the mechanism that @RobertCram developed to tell Capybara to wait until outstanding AJAX requests have all completed - that will prove to be useful, I am sure. We should make that a separate test step altogether, so we can use it as needed (e.g. Then I wait until AJAX requests complete). (kind of wonky, but since we don't expose the cute tests to the client I suppose we can get away with it).

weedySeaDragon added 17 commits June 16, 2017 13:58

update ruby_version -> 2.4.0

31414af

Merge branch 'develop' into sprint16-#143810729-UI-custom-reason-waiting

c823e06

# Conflicts: # .ruby-version

add attributes to MembershipApplication: waiting_reason & custom_reas…

44867ed

…on_text; migrate db

seed MemberAppWaitingReason with the "Other/Custom" reason (text from…

f06224f

… I18n locales)

pass locale to other_reason_name/_desc to make seeding easier

1d3a786

features & specs for setting reasons for waiting

ebb0f4f

remove assertion no longer used (removed the 'submit/save' button)

0a5dd0d

helpers: return the correct method to call based on the current local

4f48f4c

add allowed attributes to Pundit policy

441df21

refactor reasons_waiting into a parital; move to above the 'status' b…

4a3ce00

…uttons/sections

respond to a .js request for the reason waiting

aaa6ace

I18n locale entries needed

10ec808

if Seeding tests are run prior to this spec, ensure the table is clea…

e8cc76d

…red out

CSS styling

b848df6

Merge branch 'develop' into sprint16-#143810729-UI-custom-reason-waiting

4f75ecc

# Conflicts: # db/seeds.rb

fix: seed the EN name of the 'other reason' (should have passed in :e…

a041ef3

…n locale)

need to get the current text for the 'other reason' if the locale has…

a44214b

… changed (not just when page loaded)

houndci-bot reviewed Jun 21, 2017

View reviewed changes

weedySeaDragon added 3 commits June 21, 2017 13:40

Merge branch 'develop' into sprint16-#143810729-UI-custom-reason-waiting

b776f70

clean up per HoundCI suggestions

73afa63

DRY error situations

df94ef9

RobertCram reviewed Jun 22, 2017

View reviewed changes

weedySeaDragon added 6 commits June 22, 2017 12:52

features: other/custom reason comes from the locale file, is not a mo…

ba9fc60

…del (not in the db)

do not seed the 'custom/other' reason; it's not stored in the db

9b798c7

add set: select an item without any I18n

c1fbde8

CSS styling

bf2b195

return "OK" if update successful; set the text for the "other/custom.…

5bf594e

…." reason option

no .js is needed to return if action was successful

e70d877

weedySeaDragon added 2 commits June 23, 2017 15:02

other_reason placeholder stuff no longer needed in Model; remove spec…

eb3b970

…/tests

all logic for "enter custom reason.." into this javascript

608d0a7

houndci-bot reviewed Jun 23, 2017

View reviewed changes

Merge branch 'develop' into sprint16-#143810729-UI-custom-reason-waiting

2c3c0ed

patmbolger and others added 7 commits June 26, 2017 14:24

interim progress (Pat)

a73f2af

(cherry picked from commit 75f5e10)

refactored some JS

001a5d7

(cherry picked from commit 08ae9ba)

changed feature steps in accordance with id changes in view (Pat)

871fd64

(cherry picked from commit bc1d1be)

removed selenium webdriver from Gemfile (Pat)

4fdf5f5

(cherry picked from commit aa7f58b)

fixed a couple cuke steps (Pat)

cf0645a

(cherry picked from commit 2bf19ed)

fixed remaining cuke tests (Pat)

05fd86d

(cherry picked from commit 560b1ca)

changes to CSS per houndci

c0d90a8

houndci-bot reviewed Jun 26, 2017

View reviewed changes

weedySeaDragon added 3 commits June 26, 2017 15:33

POLTERGEIST fix: do not use "let" (use var instead)

6d3b704

teampoltergeist/poltergeist#895 Unfortunately parse time errors for ES6 features like *let* can be silently swallowed by PhantomJS, so you end up with no errors but also with no JS being actually run.

remove selenium (no longer needed)

e5cbbaf

update gemfile.lock

75a36d6

weedySeaDragon closed this Jun 27, 2017

		@@ -1,7 +1,12 @@
		.member_app_waiting_reasons {
		#member_app_waiting_reasons {

		@@ -0,0 +1,78 @@
		-# submit the changed reason via xhr (javascript) whenever the change from the )
		-# Do not make user press a "save" or "submit" button for just these changes.


		= button_to t('membership_applications.reject_btn'), reject_membership_application_path(@membership_application), {class: 'btn reject', disabled: !(@membership_application.may_reject?)}
		= button_to t('membership_applications.ask_applicant_for_info_btn'), need_info_membership_application_path(@membership_application), {class: 'btn need-info', disabled: !(@membership_application.may_ask_applicant_for_info?)}

		$reason-border-color: #ff7f50; // coral; picked sort of at random


		#member-app-waiting-reasons {

UI for setting the reason why SHF is waiting on an applicant #281

UI for setting the reason why SHF is waiting on an applicant #281

Conversation

weedySeaDragon commented Jun 21, 2017

Screenshots (Optional):

The reason isn't shown if the status is not 'Waiting for Applicant'

The text field for typing in the "other reason" only shows if that reason is selected

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

RobertCram commented Jun 22, 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

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

weedySeaDragon commented Jun 23, 2017 • edited Loading

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

weedySeaDragon commented Jun 26, 2017

weedySeaDragon commented Jun 27, 2017

Lessons learned:

patmbolger commented Jun 27, 2017 • edited Loading

weedySeaDragon commented Jun 23, 2017 •

edited

Loading

patmbolger commented Jun 27, 2017 •

edited

Loading