Enable setting a customized user guide link in general settings #2686

HugsDaniel · 2018-09-12T07:53:49Z

What? Why?

Closes #2593 #2631

In configuration > general settings, a field is available to add an external link for the user guide.

What should we test?

A superadmin can add a custom user guide link.
Non superadmins can't.

Release notes

An admin can now add a custom user guide link in Configuration > General Settings.

Changelog Category: Added

Dependencies

This adds customizations to Spree::Config.

Documentation updates

There could be a line added where this new feature is mentioned but I'm not sure where to put it.

HugsDaniel · 2018-09-12T07:56:57Z

app/controllers/spree/admin/general_settings_controller_decorator.rb

@@ -8,6 +8,7 @@ module GeneralSettingsEditPreferences
      def edit
        super
        @preferences_general << :bugherd_api_key
+        @preferences_general << :user_guide_link


I'm not sure about this, should I create a method below to fill @preferences_general and avoid these two lines ? Or something else ?

HugsDaniel · 2018-09-12T07:58:58Z

spec/features/admin/general_settings_spec.rb

@@ -0,0 +1,22 @@
+require 'spec_helper'


Also not sure about where to put this file. I treated it as a feature test, is that correct ?

Yes, that's fine. If you wanted to test the whole feature, you would visit the root path / and test if the URL of the link is correct. That would be more apt for a feature test.

HugsDaniel · 2018-09-12T08:06:06Z

app/models/spree/app_configuration_decorator.rb

@@ -4,6 +4,9 @@
  # we can allow to be modified in the UI by adding appropriate form
  # elements to existing or new configuration pages.

+  # General Preferences
+  preference :user_guide_link, :string, default: nil


Should I add a default user guide link (http://www.openfoodnetwork.org/platform/user-guide/ for example). Right now if no link is provided, it just links to the current page. @RachL what do you think ?

Yes, please add a default user guide to stay backwards compatible.

luisramos0

Hey, can we put this config entry on the content_configuration, below footer_about_url?
We should avoid putting more entries on this general config page if possible.

HugsDaniel · 2018-09-12T09:10:29Z

@RachL would it be ok what Luis said ? Put it in the content section

RachL · 2018-09-12T09:21:53Z

@luisramos0 do you mean here : blabla/admin/content/edit ?

HugsDaniel · 2018-09-12T09:26:14Z

I think that's what he means yes

mkllnk

Nice work. Like @luisramos0 I would like to see this link field under Settings / Content.

mkllnk · 2018-09-12T09:58:22Z

app/models/spree/app_configuration_decorator.rb

@@ -4,6 +4,9 @@
  # we can allow to be modified in the UI by adding appropriate form
  # elements to existing or new configuration pages.

+  # General Preferences
+  preference :user_guide_link, :string, default: nil


Yes, please add a default user guide to stay backwards compatible.

mkllnk · 2018-09-12T10:00:51Z

spec/features/admin/general_settings_spec.rb

@@ -0,0 +1,22 @@
+require 'spec_helper'


Yes, that's fine. If you wanted to test the whole feature, you would visit the root path / and test if the URL of the link is correct. That would be more apt for a feature test.

HugsDaniel · 2018-09-12T10:57:59Z

@mkllnk @RachL @luisramos0 I moved it to content config and tests in content_spec.rb where other content config features are tested. Thoughts ?

sauloperez · 2018-09-12T11:39:34Z

app/controllers/admin/contents_controller.rb

@@ -9,7 +9,8 @@ def edit
                              {name: I18n.t('admin.contents.edit.main_links'), preferences: [:menu_1, :menu_1_icon_name, :menu_2, :menu_2_icon_name, :menu_3, :menu_3_icon_name, :menu_4, :menu_4_icon_name, :menu_5, :menu_5_icon_name, :menu_6, :menu_6_icon_name, :menu_7, :menu_7_icon_name]},
                              {name: I18n.t('admin.contents.edit.footer_and_external_links'), preferences: [:footer_logo,
                                                             :footer_facebook_url, :footer_twitter_url, :footer_instagram_url, :footer_linkedin_url, :footer_googleplus_url, :footer_pinterest_url,
-                                                             :footer_email, :community_forum_url, :footer_links_md, :footer_about_url]}]
+                                                             :footer_email, :community_forum_url, :footer_links_md, :footer_about_url]},
+                              {name: I18n.t('admin.contents.edit.user_guide'), preferences: [:user_guide_link]}]


This #edit is getting hairy and hard to read. Honestly, I find it hard to understand what you just did. What about splitting these preference sections into objects that return each of this individual hashes and then we simply merge them into an array here? that is an approach that served me well in the past. Something like (I haven't tried it, but it should work ):

# app/models/preference_sections/user_guide_section.rb class UserGuideSection def name I18n.t('admin.contents.edit.user_guide') end def preferences [:user_guide_link] end end

# app/controllers/admin/contents_controller.rb module Admin class ContentsController < Spree::Admin::BaseController def edit @preference_sections = preference_sections.map do |preference_section| { name: preference_section.name, preferences: preference_section.preferences } end end private # Returns an array with the instances of preference sections, which all respond to #name and #preferences. def preference_sections Dir["models/preference_sections/**/*.rb"].map do |filename| basename = File.basename(filename, '.rb') basename.camelize.constantize.new end end end This makes it really easy to add, remove and change sections/preferences. And keep adding them all the time (whether they should be preferences in the UI or config settings in ofn_install it's another topic).

sauloperez · 2018-09-12T11:44:12Z

spec/features/admin/content_spec.rb

+    click_button 'Update'
+
+    expect(page).to have_content 'Your content has been successfully updated!'
+    expect(ContentConfig.user_guide_link).to eq 'http://www.openfoodnetwork.org/platform/user-guide/'


being this a feature spec, this expectation shouldn't be here because it belongs to a deeper testing level (unit? something in between?). It does not exert the UI but a class. If line 50 passes, is because ContentConfig worked. I would simply remove it because Spree::Preferences::FileConfiguration is already tested by Spree.

…de link

HugsDaniel · 2018-09-12T12:55:36Z

@sauloperez your solution works great ! It is cleaner indeed. Thoughts ?

mkllnk

Great! This is so much better. I love it when a pull request is not just changing, but improving the code. :-D

I commented on two things I would change. They are not a big deal, but it would be nice to make it perfect. :-)

mkllnk · 2018-09-13T00:08:42Z

app/controllers/admin/contents_controller.rb

+    def preference_sections
+      Dir["app/models/preference_sections/*.rb"].map do |filename|
+        basename = 'PreferenceSections::' + File.basename(filename, '.rb').camelize
+        basename.constantize.new


This is very clever. But I would prefer it more explicit. The problems I see:

From the code I can't see what is loaded. I need to go to the directory to find out.

I can't navigate to a section declaration using my IDE.

The dependency on these classes cannot be detected automatically.

One file not following the file name / class name convention breaks the code (class not found).

We cannot define the order or the sections here. They may appear random depending on the file system.

Just to give another opinion, let me answer these questions:

From the code I can't see what is loaded. I need to go to the directory to find out.

Like anything in an Object-oriented system. And that's a great benefit; you can read what the code does at an abstract level to understand how an algorithm works. You can get into detail if you need to by reading the objects' implementations.

I can't navigate to a section declaration using my IDE.

Indeed that is a con, but there's no direct reference to a particular section so checking the dir you'll see the section object you are interested in. No a big deal with so simple objects.

The dependency on these classes cannot be detected automatically.

Probably the biggest downside. We just have the call to Dir which won't be automatically detected.

One file not following the file name / class name convention breaks the code (class not found).

The file convention is being *.rb as shown in line 27. As for the class name, any name under that namespace will be picked. So unless you change this method or you don't declare the new class under the PreferenceSections namespace any will work.

We cannot define the order or the sections here. They may appear random depending on the file system.

Easy to fix by using sort_by with a method each section object declares, and that could be even more explicit than a big array as we have now.

We cannot define the order or the sections
Easy to fix by using sort_by with a method each section object declares, and that could be even more explicit than a big array as we have now.

Adding something like priority levels to the sections so that they can be sorted creates a dependency between the sections. I can't change the position of one section without knowing the positions of all other sections. A list (array) solves this very well.

The file convention is being *.rb

The file needs to have the same name as the class it contains. (This fails for class names containing API or URL unless we name the files a_p_i and u_r_l.)

They need to be in that directory. (You can't have them somewhere else and you can't have another rb file in that directory.)

These conventions are not a big deal, but I don't see any need for this flexibility here. It's a trade-off between having more code (but stupid code) and some auto-loading algorithm relying on conventions. In this case I voted for the dumb array, because it solves the sorting as well.

mkllnk · 2018-09-13T00:15:41Z

app/models/content_configuration.rb

@@ -69,4 +69,7 @@ class ContentConfiguration < Spree::Preferences::FileConfiguration
 EOS

  preference :footer_about_url, :string, default: "http://www.openfoodnetwork.org/ofn-local/open-food-network-australia/"
+
+  #User Guide
+  preference :user_guide_link, :string, default: 'http://www.openfoodnetwork.org/platform/user-guide/'


I just followed that link and the current URL is https://guide.openfoodnetwork.org/. We should use that here.

…tion

HugsDaniel · 2018-09-13T10:35:47Z

app/controllers/admin/contents_controller.rb

+    def preference_sections
+      [
+        PreferenceSections::HeaderSection.new,
+        PreferenceSections::HomePageSection.new,


@mkllnk something like this ?

Yes. I probably would have omitted the new here and called it somewhere else to reduce code duplication and separate the two concerns "list the sections" and "initialise a section to use it". But I'm happy with this.

sauloperez · 2018-09-13T11:09:41Z

app/controllers/admin/contents_controller.rb

+        PreferenceSections::GroupSignupPageSection.new,
+        PreferenceSections::MainLinksSection.new,
+        PreferenceSections::FooterAndExternalLinksSection.new,
+        PreferenceSections::UserGuideSection.new


I think this misses a bit the point and violates Open-closed principle (sorry, I think the buzzword is relevant in this context), but it is still better than it was, so 👍

Okay so it should be open to extension and closed to change, right?

View one

Closed to change means that I'm not allowed to change a file. Adding a file is an extension and good. So we just have files following a convention and auto-load them.

View two

Classes, files, directories are all data structures. Adding a section is just adding a line here. That's extension, not modification. And if you add a file to a directory, you modify that directory as well. In the end, we want to change the list of sections here and insert one particular section at a certain position. This code makes that really clear.

mkllnk · 2018-09-14T01:02:22Z

app/controllers/admin/contents_controller.rb

+    def preference_sections
+      [
+        PreferenceSections::HeaderSection.new,
+        PreferenceSections::HomePageSection.new,


Yes. I probably would have omitted the new here and called it somewhere else to reduce code duplication and separate the two concerns "list the sections" and "initialise a section to use it". But I'm happy with this.

mkllnk · 2018-09-14T01:11:25Z

app/controllers/admin/contents_controller.rb

+        PreferenceSections::GroupSignupPageSection.new,
+        PreferenceSections::MainLinksSection.new,
+        PreferenceSections::FooterAndExternalLinksSection.new,
+        PreferenceSections::UserGuideSection.new


Okay so it should be open to extension and closed to change, right?

View one

Closed to change means that I'm not allowed to change a file. Adding a file is an extension and good. So we just have files following a convention and auto-load them.

View two

Classes, files, directories are all data structures. Adding a section is just adding a line here. That's extension, not modification. And if you add a file to a directory, you modify that directory as well. In the end, we want to change the list of sections here and insert one particular section at a certain position. This code makes that really clear.

mkllnk · 2018-09-14T02:07:55Z

app/controllers/admin/contents_controller.rb

+    def preference_sections
+      Dir["app/models/preference_sections/*.rb"].map do |filename|
+        basename = 'PreferenceSections::' + File.basename(filename, '.rb').camelize
+        basename.constantize.new


We cannot define the order or the sections
Easy to fix by using sort_by with a method each section object declares, and that could be even more explicit than a big array as we have now.

Adding something like priority levels to the sections so that they can be sorted creates a dependency between the sections. I can't change the position of one section without knowing the positions of all other sections. A list (array) solves this very well.

The file convention is being *.rb

The file needs to have the same name as the class it contains. (This fails for class names containing API or URL unless we name the files a_p_i and u_r_l.)

They need to be in that directory. (You can't have them somewhere else and you can't have another rb file in that directory.)

These conventions are not a big deal, but I don't see any need for this flexibility here. It's a trade-off between having more code (but stupid code) and some auto-loading algorithm relying on conventions. In this case I voted for the dumb array, because it solves the sorting as well.

sstead · 2018-09-14T05:44:29Z

Testing Notes

Yes, the guide link is now configurable :)

https://docs.google.com/document/d/1uqQcthv0-sL4MsNrNCfKr9QdteQpJRqhsPZe_jYpjLo/edit#

RachL · 2018-09-14T12:08:38Z

@sstead did you test #2631 as well? I just want to make sure as you did not mentioned it in testing notes and both issues ended up in the same PR.

HugsDaniel force-pushed the 2593_customize_user_guide_link branch from 8a250d0 to 030730b Compare September 12, 2018 07:54

HugsDaniel commented Sep 12, 2018

View reviewed changes

HugsDaniel requested review from mkllnk, luisramos0 and sauloperez and removed request for mkllnk and luisramos0 September 12, 2018 08:03

HugsDaniel commented Sep 12, 2018

View reviewed changes

HugsDaniel mentioned this pull request Sep 12, 2018

Custom user guide link in enterprise welcome mail #2687

Closed

HugsDaniel requested a review from RachL September 12, 2018 08:10

luisramos0 reviewed Sep 12, 2018

View reviewed changes

mkllnk requested changes Sep 12, 2018

View reviewed changes

HugsDaniel force-pushed the 2593_customize_user_guide_link branch from 030730b to 82eeb6b Compare September 12, 2018 10:56

sauloperez reviewed Sep 12, 2018

View reviewed changes

sauloperez suggested changes Sep 12, 2018

View reviewed changes

Refacto edit method for content controller and enable custom user gui…

cc43d78

…de link

HugsDaniel force-pushed the 2593_customize_user_guide_link branch from 82eeb6b to cc43d78 Compare September 12, 2018 12:52

mkllnk requested changes Sep 13, 2018

View reviewed changes

mkllnk assigned HugsDaniel Sep 13, 2018

Use explicit syntax for section objects in preference_sections defini…

8dd0e01

…tion

HugsDaniel commented Sep 13, 2018

View reviewed changes

sauloperez reviewed Sep 13, 2018

View reviewed changes

sauloperez approved these changes Sep 13, 2018

View reviewed changes

mkllnk approved these changes Sep 14, 2018

View reviewed changes

mkllnk merged commit e836ac0 into openfoodfoundation:master Sep 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable setting a customized user guide link in general settings #2686

Enable setting a customized user guide link in general settings #2686

HugsDaniel commented Sep 12, 2018 •

edited

Loading

HugsDaniel Sep 12, 2018

HugsDaniel Sep 12, 2018

mkllnk Sep 12, 2018

HugsDaniel Sep 12, 2018 •

edited

Loading

mkllnk Sep 12, 2018

luisramos0 left a comment

HugsDaniel commented Sep 12, 2018

RachL commented Sep 12, 2018

HugsDaniel commented Sep 12, 2018

mkllnk left a comment •

edited

Loading

mkllnk Sep 12, 2018

mkllnk Sep 12, 2018

HugsDaniel commented Sep 12, 2018

sauloperez Sep 12, 2018

sauloperez Sep 12, 2018

HugsDaniel commented Sep 12, 2018

mkllnk left a comment

mkllnk Sep 13, 2018

sauloperez Sep 13, 2018

mkllnk Sep 14, 2018

mkllnk Sep 13, 2018

HugsDaniel Sep 13, 2018

mkllnk Sep 14, 2018

sauloperez Sep 13, 2018

mkllnk Sep 14, 2018

mkllnk Sep 14, 2018

mkllnk Sep 14, 2018

mkllnk Sep 14, 2018

sstead commented Sep 14, 2018

RachL commented Sep 14, 2018

Enable setting a customized user guide link in general settings #2686

Enable setting a customized user guide link in general settings #2686

Conversation

HugsDaniel commented Sep 12, 2018 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HugsDaniel Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisramos0 left a comment

Choose a reason for hiding this comment

HugsDaniel commented Sep 12, 2018

RachL commented Sep 12, 2018

HugsDaniel commented Sep 12, 2018

mkllnk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HugsDaniel commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HugsDaniel commented Sep 12, 2018

mkllnk 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

View one

View two

Choose a reason for hiding this comment

Choose a reason for hiding this comment

View one

View two

Choose a reason for hiding this comment

sstead commented Sep 14, 2018

RachL commented Sep 14, 2018

HugsDaniel commented Sep 12, 2018 •

edited

Loading

HugsDaniel Sep 12, 2018 •

edited

Loading

mkllnk left a comment •

edited

Loading