On off switch for din kurs events table#158297012 #542

tomurb · 2018-06-25T09:58:56Z

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

Changes proposed in this pull request:

Added show_dinkurs_events attribute to the Company model.
Added switch for new attribute for the company views' _form.html.haml, at least in case of editting.

Screenshots (Optional):

Ready for review:
@AgileVentures/shf-project-team

…8297012 adds on/off switch for Dinkurs Events

patmbolger · 2018-06-25T10:19:54Z

@tmszrbn - yes, those tests started failing very recently (I saw them yesterday). We're using a test Dinkurs account to fetch events, and I had thought that the fetched events would not change - that was a bad assumption, evidently.

I am working on a PR right now that would include a fix (short-term) for that problem.

Longer-term, we probably need to use VCR for those tests.

…8297012 failing cucumber tests

…tps://github.com/tmszrbn/shf-project into on-off-switch-for-DinKurs-events-table#158297012

tomurb · 2018-06-25T12:16:29Z

That flex display was the final commit. In retrospect I'm not sure if writing placeholder in swedish translation was the right thing to do.
Where is the rebuild button on semaphore?

patmbolger · 2018-06-25T12:42:07Z

The "rebuild" button is only visible to admins. You'll have to continue to close/reopen the PR in the meantime.

patmbolger · 2018-06-27T11:50:54Z

The PT story as written was incomplete. This feature should prevent this section of the company show view from appearing, if the switch is set to off:

However, if the user is an admin, or a member associated with this company, and there is a Dinkurs key defined for the company, we could show something like:

Showing Dinkurs events on this page has been disabled.  Edit the company to enable that.

That would serve as a reminder to the member that their events are not visible to a visitor (which they might tend to forget otherwise).

patmbolger

Please see comments.

patmbolger · 2018-06-27T11:04:19Z

db/migrate/20180624155644_add_show_dinkurs_events_column_to_company.rb

@@ -0,0 +1,5 @@
+class AddShowDinkursEventsColumnToCompany < ActiveRecord::Migration[5.1]
+  def change
+    add_column :companies, :show_dinkurs_events, :boolean


This creates a field which defaults to nil, which is OK for companies that do not use this feature yet. However, we have 5 companies in production who are already using this feature.

Here is the dinkurs-fetch log from today:

[SHF_TASK] [Load Dinkurs Events] [info] Started at 2018-06-27 01:30:07 UTC [SHF_TASK] [Load Dinkurs Events] [info] Company 24: 80 events. [SHF_TASK] [Load Dinkurs Events] [info] Company 61: 3 events. [SHF_TASK] [Load Dinkurs Events] [info] Company 89: 36 events. [SHF_TASK] [Load Dinkurs Events] [info] Company 108: 2 events. [SHF_TASK] [Load Dinkurs Events] [info] Company 116: 8 events. [SHF_TASK] [Load Dinkurs Events] [info] Company 165: 0 events. [SHF_TASK] [Load Dinkurs Events] [info] Company 214: 0 events. [SHF_TASK] [Load Dinkurs Events] [info] Finished at 2018-06-27 01:30:12 UTC. [SHF_TASK] [Load Dinkurs Events] [info] Duration: 4.94 seconds.

So, this migration has to set this value to true for any company for which dinkurs_company_id.blank? is not true (this will prevent setting the attribute to true for the two companies that have a blank string for that attribute).

I did it with after_initialize, will it be ok, to set show_dinkurs_events on true when somebody sets dinkurs_company_id?
Edit: On the second thought it doesn't seem right since they won't see that it's on while setting an id.

I would prefer doing this in the migration - it would be a one-time action and associated with the schema change, so it should be done there. Doing it in after_initialize would add unnecessary overhead at runtime.

Re your edit comment: yes, very true. And if they set a dinkurs key it does not neccessarily mean that they also want to immediately start showing events on the company page - they need to explicitly control that.

patmbolger · 2018-06-27T11:41:34Z

app/views/companies/_form.html.haml

+    = f.text_field :dinkurs_company_id, class: 'wpcf7-form-control'
+
+  .dinkurs_company_show
+    = f.label :show_dinkurs_events, "#{t('companies.show_dinkurs_events')}"


We need a tooltip for the switch - so that a user knows what it does and how to set it to on (show events) and off (hide events). In English it could be something like this:

Set this switch to on (toggle to the right) to show upcoming Dinkurs events on your company page.

patmbolger · 2018-06-27T11:51:57Z

Please note that we will also need cucumber test(s) for this (add test(s) or augment existing ones).

patmbolger · 2018-06-27T12:07:51Z

Almost forgot - you'll also have to add a test for the added DB column to company_spec.rb.

houndci-bot · 2018-06-29T16:34:38Z

app/assets/stylesheets/companies.scss

+  align-items: center;
+}
+
+label, .glyphicon {


Each selector in a comma sequence should be on its own single line

houndci-bot · 2018-06-29T16:34:38Z

app/assets/stylesheets/companies.scss

+  width: 50%;
+}
+.dinkurs_company_show {
+  display: flex;


Properties should be ordered align-items, display, flex-direction

houndci-bot · 2018-06-29T16:34:38Z

app/assets/stylesheets/companies.scss

+.dinkurs_company_id, .dinkurs_company_show {
+  width: 50%;
+}
+.dinkurs_company_show {


Selector dinkurs_company_show should be written in lowercase with hyphens

houndci-bot · 2018-06-29T16:34:39Z

app/assets/stylesheets/companies.scss

+
+.dinkurs_company_id, .dinkurs_company_show {
+  width: 50%;
+}


Rule declaration should be followed by an empty line

houndci-bot · 2018-06-29T16:34:39Z

app/assets/stylesheets/companies.scss

+  }
+}
+
+.dinkurs_company_id, .dinkurs_company_show {


Selector dinkurs_company_id should be written in lowercase with hyphens
Selector dinkurs_company_show should be written in lowercase with hyphens
Each selector in a comma sequence should be on its own single line

houndci-bot · 2018-06-29T16:34:40Z

app/assets/stylesheets/companies.scss

+    left: 4px;
+    bottom: 4px;
+    background-color: white;
+    -webkit-transition: .4s;


Avoid vendor prefixes.
.4 should be written with a leading zero as 0.4

houndci-bot · 2018-06-29T16:34:40Z

app/assets/stylesheets/companies.scss

+    width: 26px;
+    left: 4px;
+    bottom: 4px;
+    background-color: white;


Color white should be written in hexadecimal form as #ffffff
Color literals like white should only be used in variable declarations; they should be referred to via variable everywhere else.

houndci-bot · 2018-06-29T16:34:40Z

app/assets/stylesheets/companies.scss

+  transition: .4s;
+
+  &:before {
+    position: absolute;


Properties should be ordered background-color, bottom, content, height, left, position, -webkit-transition, transition, width

houndci-bot · 2018-06-29T16:34:40Z

app/assets/stylesheets/companies.scss

+  -webkit-transition: .4s;
+  transition: .4s;
+
+  &:before {


Begin pseudo elements with double colons: ::

houndci-bot · 2018-06-29T16:34:40Z

app/assets/stylesheets/companies.scss

+  bottom: 0;
+  background-color: #f4f4f8;
+  -webkit-transition: .4s;
+  transition: .4s;


.4 should be written with a leading zero as 0.4

…find it

…-events-table#158297012 [TEST] Tomek :on off switch for din kurs events table#158297012

houndci-bot · 2018-07-01T04:25:41Z

features/step_definitions/basic_steps.rb

@@ -86,7 +86,9 @@
 end

 When "I {action} the checkbox with id {capture_string}" do |action, element_id|
-  send action, element_id
+  send action, id: element_id
+rescue Capybara::ElementNotFound


Lint/Syntax: unexpected token kRESCUE

patmbolger · 2018-07-01T18:29:00Z

You have some errors with i18N. You can see the errors by running the suggested command i18n-tasks missing. This shows me:

Please try to address all (or most) of the "hound-ci" issues.
See other code comments.

patmbolger · 2018-07-01T18:23:20Z

app/views/companies/show.html.haml

@@ -47,7 +47,7 @@
    = render 'company_addresses', company: @company, user_can_edit: user_can_edit

    .col-md-12
-      - unless @company.dinkurs_company_id.blank?
+      - unless @company.dinkurs_company_id.blank? || !(current_user.member_or_admin? || @company.show_dinkurs_events)


For an admin or a member of this company, if there is a dinkurs_company_if defined, and show_dinkurs_events is false, then the view should not show anything regarding an events table.

That is, instead of seeing this:

We should just see this:

(Showing Dinkurs events on this page has been disabled. Edit the company to enable that.)

(include the parentheses).

This particular statement could be replaced with:

if ! @company.dinkurs_company_id.blank? && ! @company.show_dinkurs_events && user_can_edit

which is somewhat easier to read. (this will also prevent a member who is not associated with this company from seeing this message).

patmbolger · 2018-07-01T18:25:05Z

app/views/events/_teaser_list.html.haml

+              %td.event-start-date.text-align-center
+                = event.start_date
+        - else
+          = render 'events/empty_with_message', message: t('events.show.no_events')


With the changes recommended for company show view, we can remove this logic (that is, we don't need or want to check show_dinkurs_events in this file).

tomurb · 2018-07-02T04:46:58Z

About the Swedish translation - should I use some translator, set dummy translation or...?

patmbolger · 2018-07-02T09:25:38Z

@tmszrbn - for initial translations, I use google translate - then @thesuss corrects those as needed when she reviews the PR.

houndci-bot · 2018-07-02T13:47:01Z

app/assets/stylesheets/companies.scss

+  flex-direction: column;
+}
+
+label, 


Line contains trailing whitespace

houndci-bot · 2018-07-02T13:47:02Z

app/assets/stylesheets/companies.scss

+  width: 50%;
+}
+
+.dinkurs_company_show {


Selector dinkurs_company_show should be written in lowercase with hyphens

houndci-bot · 2018-07-02T13:47:02Z

app/assets/stylesheets/companies.scss

+  }
+}
+
+.dinkurs_company_id, 


Selector dinkurs_company_id should be written in lowercase with hyphens
Selector dinkurs_company_show should be written in lowercase with hyphens
Line contains trailing whitespace

houndci-bot · 2018-07-02T13:47:02Z

app/assets/stylesheets/companies.scss

+    box-shadow: 0 0 1px #2196f3;
+  }
+
+  input::checked + &::before {


Begin pseudo classes with a single colon: :

houndci-bot · 2018-07-02T13:47:02Z

app/assets/stylesheets/companies.scss

+  }
+
+  input::focus + & {
+    box-shadow: 0 0 1px #2196f3;


Color literals like #2196f3 should only be used in variable declarations; they should be referred to via variable everywhere else.

houndci-bot · 2018-07-02T13:47:04Z

app/assets/stylesheets/companies.scss

+
+/* The slider */
+.slider {
+  position: absolute;


Properties should be ordered background-color, bottom, cursor, height, left, position, right, top, transition

houndci-bot · 2018-07-02T13:47:04Z

app/assets/stylesheets/companies.scss

+  & input {display:none;}
+  }
+
+/* The slider */


Use // comments everywhere

houndci-bot · 2018-07-02T13:47:04Z

app/assets/stylesheets/companies.scss

+  height: 34px;
+
+  /* Hide default HTML checkbox */
+  & input {display:none;}


Unnecessary parent selector (&)
Colon after property should be followed by one space

houndci-bot · 2018-07-02T13:47:04Z

app/assets/stylesheets/companies.scss

+  width: 60px;
+  height: 34px;
+
+  /* Hide default HTML checkbox */


Use // comments everywhere

houndci-bot · 2018-07-02T13:47:05Z

app/assets/stylesheets/companies.scss

+
+ /* The switch - the box around the slider */
+.switch {
+  position: relative;


Properties should be ordered display, height, position, width

houndci-bot · 2018-07-02T14:40:24Z

app/assets/stylesheets/companies.scss

+  }
+}
+
+.dinkurs-company-id, 


Line contains trailing whitespace

houndci-bot · 2018-07-02T14:40:24Z

app/assets/stylesheets/companies.scss

+
+  // Hide default HTML checkbox
+  input {
+    display:none;


Colon after property should be followed by one space

houndci-bot · 2018-07-02T14:40:25Z

app/assets/stylesheets/companies.scss

@@ -59,7 +72,7 @@ $btn-hover-color: #7EDD8B;
    color: black;
    text-transform: none;

-    &:hover {
+    &::hover {


Begin pseudo classes with a single colon: :

houndci-bot · 2018-07-02T14:40:25Z

app/assets/stylesheets/companies.scss

+  justify-content: center;
+}
+
+.new_company {


Selector new_company should be written in lowercase with hyphens

tomurb · 2018-07-02T15:04:07Z

Sorry for spamming with fixes.
That is final, I think.

thesuss · 2018-07-02T15:03:36Z

config/locales/sv.yml

@@ -630,6 +633,8 @@ sv:
      location: Plats
      fee: Pris
      no_events: (Inga planerade händelser)
+    show_not: (Visar Dinkurs-händelser på den här sidan har inaktiverats.


Visning av Dinkurs-händelser på den här sidan har inaktiverats.

thesuss · 2018-07-02T15:03:53Z

config/locales/sv.yml

@@ -630,6 +633,8 @@ sv:
      location: Plats
      fee: Pris
      no_events: (Inga planerade händelser)
+    show_not: (Visar Dinkurs-händelser på den här sidan har inaktiverats.
+              Redigera företaget för att aktivera det.)


Redigera företaget för att aktivera visning av händelser.)

thesuss · 2018-07-02T15:04:57Z

config/locales/sv.yml

@@ -581,6 +581,8 @@ sv:
       flera län, eller "Online" om det (i huvudsak) finns på nätet
      visibility_tooltip: Hur detaljerat din adress visas för besökare (inkl kartor). Välj 'Ingen' för att inte visa din adress för besökare alls.
      org_nr_tooltip: Endast siffror (ej bindestreck)
+      dinkurs_show_tooltip: Sätt den här knappen på (växla till höger) 
+                            för att visa kommande dinkurs händelser på din företagsida.


Växla den här knappen till på (till höger)
för att visa kommande dinkurs händelser på din företagsida.

houndci-bot · 2018-07-03T05:18:09Z

app/views/companies/edit.html.haml

@@ -9,7 +9,7 @@
    = render 'business_categories/as_list',
        categories_names: @company.categories_names
 .entry-content.wpcf7
-  = render 'form'
+  = render 'form', class_name: 'edit-company' 


Line contains trailing whitespace

patmbolger · 2018-07-03T11:16:55Z

db/migrate/20180624155644_add_show_dinkurs_events_column_to_company.rb

+  def change
+    add_column :companies, :show_dinkurs_events, :boolean
+    Company.find_each do |company|
+      company.show_dinkurs_events = !company.dinkurs_company_id.blank?


Sorry - I should have caught this before .... you're not saving the updated company record here.

So, you could just add the statement company.save to the block.

It's interesting as to whether the find_each method is faster than a more direct approach, such as:

Company.where.not(dinkurs_company_id: [nil, '']).each { |c| c.update_attribute(:show_dinkurs_events, true) }

find_each will pass every company to the block. The alternative above only passes the companies with a non-blank dinkurs key, but involves more SQL.

Our DB is small enough that it really doesn't matter ... just wondering about this (your logic is fine - no change needed except for the save).

On second thought, I'd recommend changing to the SQL-select approach - that is a better approach overall and would make a significant performance difference on a large DB.

tomurb added 3 commits June 25, 2018 09:21

edit form updated

c6f46e4

Merge branch 'develop' into on-off-switch-for-DinKurs-events-table#15…

39ad9e5

…8297012 adds on/off switch for Dinkurs Events

edit form updated

bf67ce9

tomurb added 3 commits June 25, 2018 12:28

Merge branch 'develop' into on-off-switch-for-DinKurs-events-table#15…

88fb170

…8297012 failing cucumber tests

Merge branch 'on-off-switch-for-DinKurs-events-table#158297012' of ht…

ea980bc

…tps://github.com/tmszrbn/shf-project into on-off-switch-for-DinKurs-events-table#158297012

flex display for new_company form

da4f20b

tomurb closed this Jun 25, 2018

tomurb reopened this Jun 25, 2018

patmbolger requested changes Jun 27, 2018

View reviewed changes

WIP Cucumber

6345009

houndci-bot reviewed Jun 29, 2018

View reviewed changes

patmbolger and others added 3 commits June 29, 2018 18:16

added translation, check checkbox directly if selenium/chrome cannot …

3d23671

…find it

fixed cuke step

fb4cd2f

Merge pull request #1 from patmbolger/tomek-on-off-switch-for-DinKurs…

8607a4c

…-events-table#158297012 [TEST] Tomek :on off switch for din kurs events table#158297012

houndci-bot reviewed Jul 1, 2018

View reviewed changes

cucumber tests, message for member/admin, hiding from visitor

7c9ab9a

patmbolger requested changes Jul 1, 2018

View reviewed changes

houndci, i18n, hidden dinkurs events message

c89b875

houndci-bot reviewed Jul 2, 2018

View reviewed changes

houndci fixes

2307c29

houndci-bot reviewed Jul 2, 2018

View reviewed changes

houndci fixes

bc2cbc4

thesuss requested changes Jul 2, 2018

View reviewed changes

swedish translation

c0c708b

thesuss approved these changes Jul 2, 2018

View reviewed changes

houndci, show_not bolded

a119a1d

houndci-bot reviewed Jul 3, 2018

View reviewed changes

houndci

284549e

patmbolger requested changes Jul 3, 2018

View reviewed changes

migration fix

8389bf7

patmbolger approved these changes Jul 3, 2018

View reviewed changes

patmbolger merged commit 24eb704 into AgileVentures:develop Jul 3, 2018

On off switch for din kurs events table#158297012 #542

On off switch for din kurs events table#158297012 #542

Conversation

tomurb commented Jun 25, 2018 • edited

patmbolger commented Jun 25, 2018

tomurb commented Jun 25, 2018

patmbolger commented Jun 25, 2018

patmbolger commented Jun 27, 2018

patmbolger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomurb Jun 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patmbolger Jun 27, 2018 • edited

Choose a reason for hiding this comment

patmbolger commented Jun 27, 2018

patmbolger commented Jun 27, 2018

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

patmbolger commented Jul 1, 2018

patmbolger Jul 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomurb commented Jul 2, 2018

patmbolger commented Jul 2, 2018

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

tomurb commented Jul 2, 2018

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

tomurb commented Jun 25, 2018 •

edited

tomurb Jun 27, 2018 •

edited

patmbolger Jun 27, 2018 •

edited

patmbolger Jul 1, 2018 •

edited