Added fast switch option #1806

AmitJoki · 2018-03-16T18:08:23Z

Use in application.yml

  wiki_education: 'true'  # or false for P & E Dashboard

AmitJoki · 2018-03-16T18:14:27Z

@ragesoss this build will most probably fail because it needs to add a key to the application.yml and I can't because it is in the .gitignore. So could you please add the key manually?

ragesoss · 2018-03-16T18:15:42Z

app/presenters/features.rb

-  def self.disable_help?
-    ENV['disable_help'] == 'true'
+  PE.each do |method|
+    define_singleton_method method do


I would rather not do this with metaprogramming. Repeating ENV['wiki_education'] == 'true' isn't a big deal, and will keep the code extremely transparent.

I did that first but it felt weird to have the same code repeated many times. Also thought since we are whitelisting the methods, there wouldn't be anything to worry about. But your point seems fair. Have refactored like you asked.

ragesoss · 2018-03-16T18:17:35Z

@AmitJoki you can add the ENV value to config/environments/test.rb

AmitJoki · 2018-03-19T15:58:39Z

@ragesoss this build is failing due to some VCR error. Can you please rebuild this?

ragesoss · 2018-03-19T16:13:02Z

@AmitJoki that doesn't look like an error that will fix itself. I'll rebuild just to be sure, but I think it's related to this PR.

AmitJoki · 2018-03-19T17:21:06Z

@ragesoss you're right that this PR is causing it. It is failing locally for me too. But, the features.rb is replicating whatever the earlier one with lots of ENV variables was doing. So, I can't fathom why this is causing the build to break. Will check into it and if you could nitpick any cause for the error, it would be great in the meantime :)

ragesoss · 2018-03-19T17:22:24Z

The error looks like it's related to the wiki trainings feature being enabled at some place in the code where it was not previously.

ragesoss · 2018-03-19T17:30:07Z

app/presenters/features.rb

@@ -33,10 +37,6 @@ def self.enable_account_requests?
    ENV['wiki_education'] != 'true'
  end

-  def self.wiki_trainings?
-    ENV['wiki_education'] != 'true'


This was correct. Wiki trainings should be enabled with it's not wiki ed.

ragesoss · 2018-03-19T17:30:23Z

config/environments/test.rb

@@ -5,6 +5,7 @@
 ENV['open_course_creation'] = 'false'
 ENV['course_prefix'] = 'Wikipedia:Wiki_Ed'
 ENV['wiki_language'] = 'en'
+ENV['wiki_education'] = 'false'


This should be true.

You can also remove the ENV values in this file that are no longer used in Features.

I got more errors when I had that true hence put false out there. Will change and delete the redundant ENV variables.

ragesoss · 2018-03-19T17:36:28Z

spec/presenters/features_spec.rb

@@ -7,7 +7,7 @@
  describe '.wiki_ed?' do
    context 'when the url is dashboard-testing.wikiedu.org' do
      before do
-        allow(ENV).to receive(:[]).with('dashboard_url').and_return('dashboard-testing.wikiedu.org')
+        allow(ENV).to receive(:[]).with('wiki_education').and_return('true')


You can either change the 'context' description for these, or delete them altogether. After this change, they no longer test anything useful.

AmitJoki · 2018-03-20T00:46:42Z

@ragesoss see the build? I have no idea why it is failing and the logs have not been helping out either. I have returned values in accordance with your comment on the issue #1804. Can you please look into it?

ragesoss · 2018-03-20T00:55:39Z

Most or all of these are probably cases where the test environment previous was configured in a way that didn't match either of the two main environments. Features were enabled based on convenience of testing, and now those features are disabled.

For example, the RequestedAccountsController spec is written based on the default of Features.enable_account_requests? being true, even though it is false on Wiki Education production. To fix that spec, we'll need to do before { allow(Features).to receive(:enable_account_requests?).and_return(false) } at the top of the spec. The others should mostly have similar solutions.

AmitJoki · 2018-03-20T01:19:39Z

@ragesoss that explains a lot. Thanks.

AmitJoki · 2018-03-20T15:11:43Z

@ragesoss I tried your suggested method and it didn't seem to have any effect on the outcome. Hence, I've updated the tests to run conditionally.

AmitJoki · 2018-03-21T16:50:10Z

@ragesoss I have no rubocop errors for the file shown in the build logs. Can you please rebuild this one?

ragesoss · 2018-03-21T16:56:15Z

This is because of the new release of rubocop 0.54; the lastest version gets installed and run by travis. I'm fixing this today.

ragesoss

The conditional specs approach will result in never running those tests, since the test environment is set for the wiki_ed config by default. We need to tweak the tests so that they will still test everything.

ragesoss · 2018-03-28T17:19:37Z

app/presenters/features.rb

+    ENV['wiki_education'] != 'true'
+  end
+
+  def self.disable_help?


This one shows up three times.

ragesoss · 2018-03-28T17:21:52Z

app/presenters/features.rb

  end

-  def self.open_course_creation?
-    ENV['open_course_creation'] == 'true'
+  def self.disable_wiki_output?


This should keep its old definition. Wiki output is currently enabled on both dashboard (and is usually disabled in dev environments).

ragesoss · 2018-03-28T17:24:25Z

config/environments/test.rb

 ENV['default_course_type'] = 'ClassroomProgramCourse'
-ENV['open_course_creation'] = 'false'
-ENV['disable_wiki_output'] = 'false'


ragesoss · 2018-03-28T17:26:23Z

spec/controllers/article_finder_controller_spec.rb

-    it 'renders' do
-      get :index
-      expect(response.status).to eq(200)
+  if Features.enable_article_finder?


Instead of skipping these tests, you should do allow(Features).to receive(:enable_article_finder?).and_return(true) in a before block.

ragesoss · 2018-03-28T17:26:58Z

spec/controllers/requested_accounts_campaigns_controller_spec.rb

-        post :create_accounts, params: { campaign_slug: campaign.slug }
-        expect(response.status).to eq(200)
-        expect(RequestedAccount.count).to eq(0)
+      if Features.enable_account_requests?


Same thing here; use allow to turn the feature on for these tests, rather than skip the tests.

ragesoss · 2018-03-28T17:27:10Z

spec/controllers/requested_accounts_controller_spec.rb

-    let(:username) { 'username' }
-    let(:email) { 'valid@example.com' }
-
+  if Features.enable_account_requests?


ragesoss · 2018-03-28T17:29:23Z

spec/presenters/features_spec.rb

@@ -5,22 +5,13 @@

 describe Features do
  describe '.wiki_ed?' do
-    context 'when the url is dashboard-testing.wikiedu.org' do


This whole spec file can probably just be deleted, since the implementation is now trivial.

…t toggles This should fix the code review comments from PR #1806

ragesoss · 2018-05-18T21:43:02Z

thanks @AmitJoki! I finished this up and merged it.

AmitJoki · 2018-05-19T05:27:17Z

@ragesoss glad you could patch it up 😃

ragesoss reviewed Mar 16, 2018

View reviewed changes

Amit Joki added 6 commits March 19, 2018 21:59

Added fast switch option

f3f9e29

Be explicit

7f008e6

Reduced redundant code

e920ee5

Updated feature spec

4678c42

Make WikiEd true

9c0a06c

Revert the changes

528f044

AmitJoki force-pushed the One-Switch branch from 86848e9 to 528f044 Compare March 19, 2018 16:53

Change wiki_trainings

2278c4c

ragesoss reviewed Mar 19, 2018

View reviewed changes

Amit Joki added 2 commits March 19, 2018 23:18

Updated Test ENVS

748db94

Updated features spec

742feff

Updated specs to run conditionally

46cd955

Amit Joki added 2 commits March 20, 2018 23:54

Fix some more specs

ee5daee

Remove newline

be034ef

Updated language switcher specs

b0c3477

ragesoss requested changes Mar 28, 2018

View reviewed changes

ragesoss added a commit that referenced this pull request May 18, 2018

Clean up handling of feature toggles in specs, restore some indepenen…

b9cebd1

…t toggles This should fix the code review comments from PR #1806

ragesoss merged commit b0c3477 into WikiEducationFoundation:master May 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added fast switch option #1806

Added fast switch option #1806

AmitJoki commented Mar 16, 2018 •

edited

AmitJoki commented Mar 16, 2018 •

edited

ragesoss Mar 16, 2018 •

edited

AmitJoki Mar 16, 2018

ragesoss commented Mar 16, 2018

AmitJoki commented Mar 19, 2018

ragesoss commented Mar 19, 2018

AmitJoki commented Mar 19, 2018

ragesoss commented Mar 19, 2018

ragesoss Mar 19, 2018

ragesoss Mar 19, 2018

ragesoss Mar 19, 2018

AmitJoki Mar 19, 2018

ragesoss Mar 19, 2018

AmitJoki commented Mar 20, 2018

ragesoss commented Mar 20, 2018 •

edited

AmitJoki commented Mar 20, 2018

AmitJoki commented Mar 20, 2018 •

edited

AmitJoki commented Mar 21, 2018

ragesoss commented Mar 21, 2018

ragesoss left a comment

ragesoss Mar 28, 2018

ragesoss Mar 28, 2018

ragesoss Mar 28, 2018

ragesoss Mar 28, 2018

ragesoss Mar 28, 2018

ragesoss Mar 28, 2018

ragesoss Mar 28, 2018

ragesoss commented May 18, 2018

AmitJoki commented May 19, 2018

Added fast switch option #1806

Added fast switch option #1806

Conversation

AmitJoki commented Mar 16, 2018 • edited

AmitJoki commented Mar 16, 2018 • edited

ragesoss Mar 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragesoss commented Mar 16, 2018

AmitJoki commented Mar 19, 2018

ragesoss commented Mar 19, 2018

AmitJoki commented Mar 19, 2018

ragesoss commented Mar 19, 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

AmitJoki commented Mar 20, 2018

ragesoss commented Mar 20, 2018 • edited

AmitJoki commented Mar 20, 2018

AmitJoki commented Mar 20, 2018 • edited

AmitJoki commented Mar 21, 2018

ragesoss commented Mar 21, 2018

ragesoss 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

ragesoss commented May 18, 2018

AmitJoki commented May 19, 2018

AmitJoki commented Mar 16, 2018 •

edited

AmitJoki commented Mar 16, 2018 •

edited

ragesoss Mar 16, 2018 •

edited

ragesoss commented Mar 20, 2018 •

edited

AmitJoki commented Mar 20, 2018 •

edited