[DEV ONLY] Weedy - sprint16 #143810729 ui custom reason waiting by patmbolger · Pull Request #1 · weedySeaDragon/shf-project

patmbolger · 2017-06-24T17:36:14Z

This "PR" compares changes in my feature branch against the baseline feature branch from Ashley.

Related to the story in the title.

Changes proposed in this pull request:
1.
2.
3.

Screenshots (Optional):

patmbolger · 2017-06-24T17:43:10Z

+  .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


I removed the form as it was not needed and also caused bugs. (in your feature branch, enter a custom reason and hit "enter" key - you will see an error alert - this is because after the AJAX stuff happens, the form is also submitted to the controller action (not what we want).

I added the "custom reason" item to the select_tag collection. I hacked it up here but it should probably be returned from a helper method. (also, note the ugly way that I am finding the current selected value for the select collection. This can be removed when we refactor the custom reason into the AdminOnly::MemberAppWaitingReason model).

I also added the currently-selected option to the options collection.

I refactored the "onchange" stuff to the DOM-ready function.

I also simplified the ID for the select list and the text_field. (this make for easier read of the code, IMO)

patmbolger · 2017-06-24T17:44:15Z

 :javascript

-  let reasons_list = document.getElementById('membership_application_member_app_waiting_reasons_id');  // the select HTML element (list)
+  let reasons_list = $('#member_app_waiting_reasons');  // the select HTML element (list)


You're using both straight JSAPI as well as Jquery - thought it best to just use Jquery throughput.

patmbolger · 2017-06-24T17:44:51Z


  function selected_is_customOtherReason() {
-    return reasons_list.value === other_reason_option.value;
+    return $('#member_app_waiting_reasons option:selected').text() === other_reason_option.text;


Your statement did not work for me - it could be because I converted to query(?).

patmbolger · 2017-06-24T17:47:27Z


  let initialize = (function() {
-    reasons_list.options.add(other_reason_option);  // add the other_reason to the list
+    // reasons_list.options.add(other_reason_option);  // add the other_reason to the list


Not needed since I added the option directly to the options passed to select_tag.

Also, this statement was causing a test failure when I used poltergeist as the web driver. I then tried using Selenium and it worked. I have seen poltergeist have trouble with some JS code before, and Selenium generally does a better job with tricky JS.

patmbolger · 2017-06-24T17:48:17Z

+    // reasons_list.options.add(other_reason_option);  // add the other_reason to the list
    hideOrShowCustomReasonElements();
+    $('#member_app_waiting_reasons').on('change', changed_reason);
+    $('#custom_reason_text').on('change', changed_custom_text)


Here are the refactored 'onchange' triggers.

patmbolger · 2017-06-24T17:49:34Z

+  create_table "users", id: :bigserial, force: :cascade do |t|
    t.string   "email",                  default: "",    null: false
    t.string   "encrypted_password",     default: "",    null: false
    t.string   "reset_password_token"


I'm not sure how these differences occurred (did not change any migration file). Hopefully they are innocuous ...

patmbolger · 2017-06-24T17:50:17Z


    # it is important that this statement is first so that tables are empty, so that things will be seeded
-    Given the system is seeded with initial data
+    # Given the system is seeded with initial data


Since we now add the "custom reason" selection directly (no longer from the DB) this background step is not needed.

patmbolger · 2017-06-24T17:50:53Z



-  @admin
+  @admin @javascript


I can't see how the tag @admin also pulled in poltergeist, so added that explicitly.

patmbolger · 2017-06-24T17:51:29Z

+Before('@selenium') do
+  # For JS-driven tests that Poltergeist does not handle cleanly.
+  Capybara.javascript_driver = :selenium
+end


This is a hold-over from when I was testing with Selenium. We don't need this change.

patmbolger · 2017-06-24T17:55:33Z

@weedySeaDragon - see comments in code tab.

I still have 4 failures occurring in

cucumber features/membership-application-status/

Those all come from admin-sets-custom-waiting-reason.feature:

patmbolger · 2017-06-24T18:04:21Z

I just fixed one of those failing tests. Now we have:

patmbolger · 2017-06-24T18:06:47Z

I think one or two of the failures could be due to this kind of step:

And I should not see t("admin_only.member_app_waiting_reasons.other_custom_reason")

The step method for this is:

And(/^I should not see "([^"]*)"$/) do |content|
  expect(page).not_to have_content content
end

I think that this step will fail because even though the content may not be visible, it is still on the page. We need a step that checks for visibility.

patmbolger

All tests now pass.

patmbolger · 2017-06-25T12:49:14Z


 group :test do
  gem 'poltergeist'
+  gem 'selenium-webdriver'


I had 4 remaining failing cuke tests that will (so far) only run successfully when using Selenium.

patmbolger · 2017-06-25T12:49:34Z

  rubyzip (>= 1.2.1)
  sanitize
  sass-rails (~> 5.0)
+  selenium-webdriver


Need this gem for Selenium driver.

* Bump rack from 2.2.3 to 2.2.3.1 Bumps [rack](https://github.com/rack/rack) from 2.2.3 to 2.2.3.1. - [Release notes](https://github.com/rack/rack/releases) - [Changelog](https://github.com/rack/rack/blob/main/CHANGELOG.md) - [Commits](rack/rack@2.2.3...2.2.3.1) --- updated-dependencies: - dependency-name: rack dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> * semaphore: install missing libpng12-0 library (try #1) * use wget to install libpng-12 manually Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ashley Engelund (weedySeaDragon @ github) <ashley@ashleycaroline.com>

patmbolger added 4 commits June 24, 2017 08:46

interim progress

75f5e10

refactored some JS

08ae9ba

changed feature steps in accordance with id changes in view

bc1d1be

removed selenium webdriver from Gemfile

aa7f58b

patmbolger commented Jun 24, 2017

View reviewed changes

fixed a couple cuke steps

2bf19ed

fixed remaining cuke tests

560b1ca

patmbolger commented Jun 25, 2017

View reviewed changes

patmbolger added 3 commits June 26, 2017 18:15

changed JS code, converted back to poltergeist from selenium

d300dde

removed selenium

4dd87d2

more JS refactor; remove wait from cuke test

31a1bdb

patmbolger force-pushed the weedy-sprint16-#143810729-UI-custom-reason-waiting branch from ecc7b69 to 31a1bdb Compare June 27, 2017 11:52

Conversation

patmbolger commented Jun 24, 2017

Uh oh!

patmbolger Jun 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patmbolger Jun 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patmbolger commented Jun 24, 2017

Uh oh!

patmbolger commented Jun 24, 2017

Uh oh!

patmbolger commented Jun 24, 2017

Uh oh!

patmbolger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

patmbolger Jun 24, 2017 •

edited

Loading

patmbolger Jun 24, 2017 •

edited

Loading