From c23bb6e4f2ddd3c53be109277ab8ca9ddab3af4d Mon Sep 17 00:00:00 2001 From: Liz Conlan Date: Fri, 21 Jun 2019 10:33:15 +0100 Subject: [PATCH 001/124] Enable Rails 5.1 testing on CI --- Gemfile | 2 +- Gemfile.rails_next.lock | 85 ++++++++++--------- .../alaveteli_features.gemspec | 2 +- 3 files changed, 45 insertions(+), 44 deletions(-) diff --git a/Gemfile b/Gemfile index a39b2ea821..89693cd597 100644 --- a/Gemfile +++ b/Gemfile @@ -83,7 +83,7 @@ def rails_upgrade? %w[1 true].include?(ENV['RAILS_UPGRADE']) end -gem 'rails', '5.0.7.2' +gem 'rails', rails_upgrade? ? '5.1.7' : '5.0.7.2' gem 'pg', '~> 0.20.0' diff --git a/Gemfile.rails_next.lock b/Gemfile.rails_next.lock index 41b3d6a365..0520eb6bce 100644 --- a/Gemfile.rails_next.lock +++ b/Gemfile.rails_next.lock @@ -39,44 +39,44 @@ PATH flipper flipper-active_record mime-types (< 3.0.0) - rails (~> 5.0.7) + rails (~> 5.1.7) GEM remote: https://rubygems.org/ specs: - actioncable (5.0.7.2) - actionpack (= 5.0.7.2) - nio4r (>= 1.2, < 3.0) + actioncable (5.1.7) + actionpack (= 5.1.7) + nio4r (~> 2.0) websocket-driver (~> 0.6.1) - actionmailer (5.0.7.2) - actionpack (= 5.0.7.2) - actionview (= 5.0.7.2) - activejob (= 5.0.7.2) + actionmailer (5.1.7) + actionpack (= 5.1.7) + actionview (= 5.1.7) + activejob (= 5.1.7) mail (~> 2.5, >= 2.5.4) rails-dom-testing (~> 2.0) - actionpack (5.0.7.2) - actionview (= 5.0.7.2) - activesupport (= 5.0.7.2) + actionpack (5.1.7) + actionview (= 5.1.7) + activesupport (= 5.1.7) rack (~> 2.0) - rack-test (~> 0.6.3) + rack-test (>= 0.6.3) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.0.2) - actionview (5.0.7.2) - activesupport (= 5.0.7.2) + actionview (5.1.7) + activesupport (= 5.1.7) builder (~> 3.1) - erubis (~> 2.7.0) + erubi (~> 1.4) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.0.3) - activejob (5.0.7.2) - activesupport (= 5.0.7.2) + activejob (5.1.7) + activesupport (= 5.1.7) globalid (>= 0.3.6) - activemodel (5.0.7.2) - activesupport (= 5.0.7.2) - activerecord (5.0.7.2) - activemodel (= 5.0.7.2) - activesupport (= 5.0.7.2) - arel (~> 7.0) - activesupport (5.0.7.2) + activemodel (5.1.7) + activesupport (= 5.1.7) + activerecord (5.1.7) + activemodel (= 5.1.7) + activesupport (= 5.1.7) + arel (~> 8.0) + activesupport (5.1.7) concurrent-ruby (~> 1.0, >= 1.0.2) i18n (>= 0.7, < 2) minitest (~> 5.1) @@ -86,7 +86,7 @@ GEM annotate (2.7.4) activerecord (>= 3.2, < 6.0) rake (>= 10.4, < 13.0) - arel (7.1.4) + arel (8.0.0) ast (2.4.0) bcrypt (3.1.12) bindex (0.7.0) @@ -156,6 +156,7 @@ GEM diff-lcs (1.3) docile (1.3.1) dynamic_form (1.1.4) + erubi (1.8.0) erubis (2.7.0) eventmachine (1.2.7) exception_notification (4.1.1) @@ -267,21 +268,21 @@ GEM rack rack-ssl (1.4.1) rack - rack-test (0.6.3) - rack (>= 1.0) + rack-test (1.1.0) + rack (>= 1.0, < 3) rack-utf8_sanitizer (1.3.2) rack (>= 1.0, < 3.0) - rails (5.0.7.2) - actioncable (= 5.0.7.2) - actionmailer (= 5.0.7.2) - actionpack (= 5.0.7.2) - actionview (= 5.0.7.2) - activejob (= 5.0.7.2) - activemodel (= 5.0.7.2) - activerecord (= 5.0.7.2) - activesupport (= 5.0.7.2) + rails (5.1.7) + actioncable (= 5.1.7) + actionmailer (= 5.1.7) + actionpack (= 5.1.7) + actionview (= 5.1.7) + activejob (= 5.1.7) + activemodel (= 5.1.7) + activerecord (= 5.1.7) + activesupport (= 5.1.7) bundler (>= 1.3.0) - railties (= 5.0.7.2) + railties (= 5.1.7) sprockets-rails (>= 2.0.0) rails-controller-testing (1.0.2) actionpack (~> 5.x, >= 5.0.1) @@ -295,9 +296,9 @@ GEM rails-i18n (5.1.3) i18n (>= 0.7, < 2) railties (>= 5.0, < 6) - railties (5.0.7.2) - actionpack (= 5.0.7.2) - activesupport (= 5.0.7.2) + railties (5.1.7) + actionpack (= 5.1.7) + activesupport (= 5.1.7) method_source rake (>= 0.8.7) thor (>= 0.18.1, < 2.0) @@ -414,7 +415,7 @@ GEM hashdiff websocket-driver (0.6.5) websocket-extensions (>= 0.1.0) - websocket-extensions (0.1.3) + websocket-extensions (0.1.4) will_paginate (3.1.6) xapian-full-alaveteli (1.2.21.1) xml-simple (1.1.5) @@ -481,7 +482,7 @@ DEPENDENCIES rack (~> 2.0.0) rack-ssl (~> 1.4.0) rack-utf8_sanitizer (~> 1.3.0) - rails (= 5.0.7.2) + rails (= 5.1.7) rails-controller-testing rails-i18n (~> 5.1.0) recaptcha (~> 4.9.0, < 4.10.0) diff --git a/gems/alaveteli_features/alaveteli_features.gemspec b/gems/alaveteli_features/alaveteli_features.gemspec index 5bdc10ffbc..853e441dc4 100644 --- a/gems/alaveteli_features/alaveteli_features.gemspec +++ b/gems/alaveteli_features/alaveteli_features.gemspec @@ -22,7 +22,7 @@ Gem::Specification.new do |spec| spec.test_files = spec.files.grep(%r{^(test|spec|features)/}) spec.require_paths = ["lib"] - spec.add_dependency "rails", "~> 5.0.7" + spec.add_dependency "rails", rails_upgrade? ? "~> 5.1.7" : "~> 5.0.7" spec.add_dependency "flipper" spec.add_dependency "flipper-active_record" # Mime types 3 needs Ruby 2.0.0 or greater, but we need to support 1.9.3 so From 6b5076d00773e03a7589823dd562f6b4aa714c7c Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Mon, 12 Aug 2019 10:55:48 +0100 Subject: [PATCH 002/124] Fix rendering of locale switcher Moving the before action callback before the protect_from_forgery action ensure 'locale' instance variable is always defined. This is used on every page, including when exceptions are rendered in production mode. Fixes #4479 --- app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0e983fd841..e753cae45c 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -14,6 +14,7 @@ class PermissionDenied < StandardError end class RouteNotFound < StandardError end + before_action :collect_locales protect_from_forgery :if => :user?, :with => :exception skip_before_action :verify_authenticity_token, :unless => :user? @@ -37,7 +38,6 @@ class RouteNotFound < StandardError before_action :session_remember_me before_action :set_vary_header before_action :validate_session_timestamp - before_action :collect_locales after_action :persist_session_timestamp def set_vary_header From 26af010dec7af2a6c06ebe40c0245973a6056b8f Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Mon, 12 Aug 2019 11:15:31 +0100 Subject: [PATCH 003/124] Drop token check when dismissing announcements No need to protect this action as it doesn't handle any thing sensitive. Fixes #4495 --- app/controllers/announcements_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/announcements_controller.rb b/app/controllers/announcements_controller.rb index 6cf3362a88..cfbf87eea6 100644 --- a/app/controllers/announcements_controller.rb +++ b/app/controllers/announcements_controller.rb @@ -1,4 +1,6 @@ class AnnouncementsController < ApplicationController + skip_before_action :verify_authenticity_token, only: [:destroy] + def destroy if announcement store_dismissal_in_session unless dismissal.save From e5feeca5dabf024234755956fbb75596741c183c Mon Sep 17 00:00:00 2001 From: Liz Conlan Date: Thu, 13 Jun 2019 12:59:52 +0100 Subject: [PATCH 004/124] Use strings instead of classes in `has_many` Passing classes brings up a deprecation warning in Rails 5.1: Passing a class to the `class_name` is deprecated and will raise an ArgumentError in Rails 5.2. It eagerloads more classes than necessary and potentially creates circular dependencies. Please pass the class name as a string --- app/models/user.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index d561966625..d8bc986e14 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -96,7 +96,7 @@ class User < ApplicationRecord -> { order('created_at desc') }, :inverse_of => :user, :dependent => :destroy, - :class_name => AlaveteliPro::DraftInfoRequestBatch + :class_name => 'AlaveteliPro::DraftInfoRequestBatch' has_many :request_classifications, :inverse_of => :user, :dependent => :destroy @@ -106,7 +106,7 @@ class User < ApplicationRecord has_many :request_summaries, :inverse_of => :user, :dependent => :destroy, - :class_name => AlaveteliPro::RequestSummary + :class_name => 'AlaveteliPro::RequestSummary' has_many :notifications, :inverse_of => :user, :dependent => :destroy From 9bc4d15aa6c65fc00bf8c1fbae5078e60fa75742 Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Wed, 14 Aug 2019 17:05:15 +0100 Subject: [PATCH 005/124] Ignore /vendor/data c10f6c6d005cd1dc1404cda46f5031bfe1696127 set the download location of the geoip database to `APP_ROOT/vendor/data`, so this ignores that as we don't want to distribute the database with alaveteli directly. --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index ecc4965fbf..85617ce4f4 100644 --- a/.gitignore +++ b/.gitignore @@ -40,3 +40,4 @@ public/google*.html public/wordpress tmp/ vendor/bundle/ +vendor/data/ From 9a6be03e84504d2acb0f2867e2ba9f01cd8f2ab3 Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Wed, 14 Aug 2019 17:24:41 +0100 Subject: [PATCH 006/124] Re-add develop changelog section --- doc/CHANGES.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/doc/CHANGES.md b/doc/CHANGES.md index 12150f57df..1357b4f392 100644 --- a/doc/CHANGES.md +++ b/doc/CHANGES.md @@ -1,3 +1,11 @@ +# develop + +## Highlighted Features + +## Upgrade Notes + +### Changed Templates + # 0.35.0.0 ## Highlighted Features From 92ba759dc37a1365323381bd4a60a07785ab6345 Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Thu, 8 Aug 2019 16:07:16 +0100 Subject: [PATCH 007/124] Minor code cleanup * Hash style * Quote style * Brace padding * Line spacing * DSL Parenthesis (don't need them for select tag) --- app/views/admin_incoming_message/edit.html.erb | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/app/views/admin_incoming_message/edit.html.erb b/app/views/admin_incoming_message/edit.html.erb index b349ba033f..b47ae8c0c5 100644 --- a/app/views/admin_incoming_message/edit.html.erb +++ b/app/views/admin_incoming_message/edit.html.erb @@ -1,26 +1,28 @@ -<%= render :partial => 'intro', :locals => {:incoming_message => @incoming_message } %> -<%= render :partial => 'actions', :locals => { :incoming_message => @incoming_message } %> +<%= render partial: 'intro', locals: { incoming_message: @incoming_message } %> +<%= render partial: 'actions', locals: { incoming_message: @incoming_message } %> +
Prominence - <%= form_tag admin_incoming_message_path(@incoming_message), :method => 'put', :class => "form form-inline" do %> + <%= form_tag admin_incoming_message_path(@incoming_message), method: 'put', class: 'form form-inline' do %>
- + +
- <%= select('incoming_message', "prominence", IncomingMessage.prominence_states) %> + <%= select 'incoming_message', 'prominence', IncomingMessage.prominence_states %>
+
- <%= text_area "incoming_message", "prominence_reason", :rows => 5, :class => "span6" %> + <%= text_area 'incoming_message', 'prominence_reason', rows: 5, class: 'span6' %>
- <%= submit_tag 'Save', :class => "btn" %> + <%= submit_tag 'Save', class: 'btn' %>
- <% end %>
From 392a2dad44d5c8f38898a8dd8759933cb3bbfcdf Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Thu, 8 Aug 2019 16:08:36 +0100 Subject: [PATCH 008/124] List FOI Attachments on admin page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When handling data breaches we need to find the hexdigest of the attachment to ensure it gets purged from all caches / backups etc. Its quite hard to do this – even from the console – and any console commands are prone to error. Its easier to just show the associated attachments on the admin interface, along with some less user-friendly information that's useful for admin purposes. --- .../_foi_attachments.html.erb | 30 +++++++++++++++++++ .../admin_incoming_message/edit.html.erb | 3 ++ doc/CHANGES.md | 2 ++ 3 files changed, 35 insertions(+) create mode 100644 app/views/admin_incoming_message/_foi_attachments.html.erb diff --git a/app/views/admin_incoming_message/_foi_attachments.html.erb b/app/views/admin_incoming_message/_foi_attachments.html.erb new file mode 100644 index 0000000000..5e7a865e47 --- /dev/null +++ b/app/views/admin_incoming_message/_foi_attachments.html.erb @@ -0,0 +1,30 @@ +
+ Attachments + + <% if foi_attachments.any? %> + + + + + + + + + + + + <% foi_attachments.each do |attachment| %> + + + + + + + + <% end %> + +
IDFilenameContent TypeHexdigestDisplay Size
<%= attachment.id %><%= attachment.filename %><%= attachment.content_type %><%= attachment.hexdigest %><%= attachment.display_size %>
+ <% else %> +

No attachments.

+ <% end %> +
diff --git a/app/views/admin_incoming_message/edit.html.erb b/app/views/admin_incoming_message/edit.html.erb index b47ae8c0c5..02832c652e 100644 --- a/app/views/admin_incoming_message/edit.html.erb +++ b/app/views/admin_incoming_message/edit.html.erb @@ -26,3 +26,6 @@ <% end %> + +<%= render partial: 'foi_attachments', + locals: { foi_attachments: @incoming_message.foi_attachments } %> diff --git a/doc/CHANGES.md b/doc/CHANGES.md index 1357b4f392..29998fe62b 100644 --- a/doc/CHANGES.md +++ b/doc/CHANGES.md @@ -2,6 +2,8 @@ ## Highlighted Features +* Show incoming message attachments in admin interface (Gareth Rees) + ## Upgrade Notes ### Changed Templates From aedc9080b49de43721876ce773bad73f8dee9e10 Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Wed, 14 Aug 2019 17:51:33 +0100 Subject: [PATCH 009/124] Add some hound checks --- .ruby-style.yml | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/.ruby-style.yml b/.ruby-style.yml index e37395e655..5c280e0e49 100644 --- a/.ruby-style.yml +++ b/.ruby-style.yml @@ -139,7 +139,7 @@ Layout/IndentArray: Description: >- Checks the indentation of the first element in an array literal. - Enabled: false + Enabled: true Layout/IndentAssignment: Description: >- @@ -149,7 +149,7 @@ Layout/IndentAssignment: Layout/IndentHash: Description: 'Checks the indentation of the first key in a hash literal.' - Enabled: false + Enabled: true Layout/LeadingCommentSpace: Description: 'Comments should start with a space.' @@ -300,7 +300,7 @@ Layout/SpaceInsideArrayLiteralBrackets: Layout/SpaceInsideReferenceBrackets: Description: 'Checks the spacing inside referential brackets.' - Enabled: false + Enabled: true Layout/SpaceInsideHashLiteralBraces: Description: "Use spaces inside hash literal braces - or don't." @@ -312,12 +312,12 @@ Layout/SpaceInsideHashLiteralBraces: Layout/SpaceInsideParens: Description: 'No spaces after ( or before ).' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-spaces-braces' - Enabled: false + Enabled: true Layout/SpaceInsideRangeLiteral: Description: 'No spaces inside range literals.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-space-inside-range-literals' - Enabled: false + Enabled: true Layout/SpaceInsideStringInterpolation: Description: 'Checks for padding/surrounding spaces inside string interpolation.' @@ -348,15 +348,15 @@ Layout/ConditionPosition: Checks for condition placed in a confusing position relative to the keyword. StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#same-line-condition' - Enabled: false + Enabled: true Layout/DefEndAlignment: Description: 'Align ends corresponding to defs correctly.' - Enabled: false + Enabled: true Layout/EndAlignment: Description: 'Align ends correctly.' - Enabled: false + Enabled: true Style/Alias: Description: 'Use alias instead of alias_method.' @@ -456,7 +456,7 @@ Style/CommentAnnotation: Checks formatting of special comments (TODO, FIXME, OPTIMIZE, HACK, REVIEW). StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#annotate-keywords' - Enabled: false + Enabled: true Style/ConditionalAssignment: Description: >- @@ -509,7 +509,7 @@ Style/EmptyCaseCondition: Style/EmptyLiteral: Description: 'Prefer literals to Array.new/Hash.new/String.new.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#literal-array-hash' - Enabled: false + Enabled: true Style/Encoding: Description: 'Use UTF-8 as the source file encoding.' From d2fe9c7bbb647356da8ab0d6306e230e8a31ecb7 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Thu, 29 Aug 2019 16:17:24 +0100 Subject: [PATCH 010/124] Allow empty lines around block body in spec files We don't currently want to run this cop on spec files as it goes against our current style for describe/context/it blocks. We may switch to this style in the future but for now it will return too many warnings when reviewing PRs. --- .ruby-style.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.ruby-style.yml b/.ruby-style.yml index 5c280e0e49..46fac80cf8 100644 --- a/.ruby-style.yml +++ b/.ruby-style.yml @@ -91,6 +91,8 @@ Layout/EmptyLinesAroundBlockBody: Description: "Keeps track of empty lines around block bodies." Enabled: true EnforcedStyle: no_empty_lines + Exclude: + - 'spec/**/*' Layout/EmptyLinesAroundClassBody: Description: "Keeps track of empty lines around class bodies." From df8af8a2ab93e2dda4f98ea34acbfe0f96a6fae4 Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Fri, 30 Aug 2019 08:57:52 +0100 Subject: [PATCH 011/124] Download geoip data on install c10f6c6d005cd1dc1404cda46f5031bfe1696127 introduces a rake task to download geoip data. Once configured correctly, 42d0c1b0ec2c4a06934dc72b46835dd18aeb9d4a handles keeping it updated. This commit ensures that the data exists on the first install of the app. --- script/rails-deploy-before-down | 2 ++ 1 file changed, 2 insertions(+) diff --git a/script/rails-deploy-before-down b/script/rails-deploy-before-down index 6bf463c82b..28d8bd2b38 100755 --- a/script/rails-deploy-before-down +++ b/script/rails-deploy-before-down @@ -119,6 +119,8 @@ bundle exec rake submodules:check bundle exec rake themes:install +bundle exec rake geoip:download_data + if [ "$OPTION_STAGING_SITE" = "0" ] then bundle exec rake assets:precompile && From 86b101533c80ebee23f979577b609898d94162eb Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Mon, 2 Sep 2019 16:37:14 +0100 Subject: [PATCH 012/124] Enable hound checks Nothing really controversial here. --- .ruby-style.yml | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/.ruby-style.yml b/.ruby-style.yml index 46fac80cf8..08ddbb76e9 100644 --- a/.ruby-style.yml +++ b/.ruby-style.yml @@ -431,7 +431,7 @@ Style/ClassAndModuleChildren: Style/ClassCheck: Description: 'Enforces consistent use of `Object#is_a?` or `Object#kind_of?`.' - Enabled: false + Enabled: true Style/ClassMethods: Description: 'Use self when defining module/class methods.' @@ -441,7 +441,7 @@ Style/ClassMethods: Style/ClassVars: Description: 'Avoid the use of class variables.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-class-vars' - Enabled: false + Enabled: true Style/ColonMethodCall: Description: 'Do not use :: for method call.' @@ -521,12 +521,12 @@ Style/Encoding: Style/EndBlock: Description: 'Avoid the use of END blocks.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-END-blocks' - Enabled: false + Enabled: true Style/EvenOdd: Description: 'Favor the use of Fixnum#even? && Fixnum#odd?' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#predicate-methods' - Enabled: false + Enabled: true Style/FrozenStringLiteralComment: Description: >- @@ -547,13 +547,13 @@ Style/For: Style/FormatString: Description: 'Enforce the use of Kernel#sprintf, Kernel#format or String#%.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#sprintf' - Enabled: false + Enabled: true Style/GlobalVars: Description: 'Do not introduce global variables.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#instance-vars' Reference: 'http://www.zenspider.com/Languages/Ruby/QuickRef.html' - Enabled: false + Enabled: true Style/GuardClause: Description: 'Check for conditionals that can be replaced with guard clauses' @@ -601,7 +601,7 @@ Style/IdenticalConditionalBranches: Style/InfiniteLoop: Description: 'Use Kernel#loop for infinite loops.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#infinite-loop' - Enabled: false + Enabled: true Style/Lambda: Description: 'Use the new lambda literal syntax for single-line blocks.' @@ -611,7 +611,7 @@ Style/Lambda: Style/LambdaCall: Description: 'Use lambda.call(...) instead of lambda.(...).' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#proc-call' - Enabled: false + Enabled: true Style/LineEndConcatenation: Description: >- @@ -629,7 +629,7 @@ Style/MethodDefParentheses: Checks if the method definitions have or don't have parentheses. StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#method-parens' - Enabled: false + Enabled: true Style/ModuleFunction: Description: 'Checks for usage of `extend self` in modules.' @@ -667,7 +667,7 @@ Style/NegatedIf: Style/NegatedWhile: Description: 'Favor until over while for negative conditions.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#until-for-negatives' - Enabled: false + Enabled: true Style/NestedModifier: Description: 'Avoid using nested modifiers.' From 973676a8a5cb12daf85c36ee468444bd2a25e074 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Thu, 29 Aug 2019 15:42:10 +0100 Subject: [PATCH 013/124] Refactor set_gettext_locale Using #update_column prevents the User model callbacks from being triggered when updating the user's locale attribute. This will help by preventing User#update_pro_account callback from being called when creating users in spec setup steps. --- app/controllers/application_controller.rb | 37 +++++++++------------ spec/controllers/request_controller_spec.rb | 2 +- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index e753cae45c..1e4f0213a0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -68,30 +68,25 @@ def long_cache # required due to the I18nProxy used in Rails to trigger the # lookup_context and expire the template cache def set_gettext_locale + params_locale = params[:locale] + if AlaveteliConfiguration.include_default_locale_in_urls == false - params_locale = params.fetch(:locale) do - AlaveteliLocalization.default_locale - end - else - params_locale = params[:locale] - end - browser_locale = if AlaveteliConfiguration.use_default_browser_language - request.env['HTTP_ACCEPT_LANGUAGE'] - else - nil + params_locale ||= AlaveteliLocalization.default_locale end - AlaveteliLocalization.set_session_locale(params_locale, - session[:locale], - cookies[:locale], - browser_locale) - # set the currrent locale to the requested_locale - session[:locale] = AlaveteliLocalization.locale - if !@user.nil? - if @user.locale != AlaveteliLocalization.locale - @user.locale = AlaveteliLocalization.locale - @user.save! - end + + if AlaveteliConfiguration.use_default_browser_language + browser_locale = request.env['HTTP_ACCEPT_LANGUAGE'] end + + locale = AlaveteliLocalization.set_session_locale( + params_locale, session[:locale], cookies[:locale], browser_locale + ) + + # set the current locale to the requested_locale + session[:locale] = locale + + # ensure current user locale attribute is up-to-date + current_user.update_column(:locale, locale) if current_user end # Help work out which request causes RAM spike. diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb index fee0248a2e..564460190c 100644 --- a/spec/controllers/request_controller_spec.rb +++ b/spec/controllers/request_controller_spec.rb @@ -2050,7 +2050,7 @@ def send_request describe RequestController, "when making a new request" do before do - @user = mock_model(User, :id => 3481, :name => 'Testy') + @user = mock_model(User, id: 3481, name: 'Testy').as_null_object allow(@user).to receive(:get_undescribed_requests).and_return([]) allow(@user).to receive(:can_file_requests?).and_return(true) allow(@user).to receive(:locale).and_return("en") From 492f4b1c5111f51b8db93f705787c9cace1c564f Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Thu, 29 Aug 2019 15:33:38 +0100 Subject: [PATCH 014/124] Add spec before blocks to disable pro_batch_access Explicitly disabled in specs which are checking for this feature to be disabled. --- .../batch_request_authority_searches_controller_spec.rb | 8 ++++++++ spec/integration/alaveteli_pro/request_creation_spec.rb | 2 ++ 2 files changed, 10 insertions(+) diff --git a/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb b/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb index 0378a2877a..84ae7af0d4 100644 --- a/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb @@ -148,6 +148,10 @@ context "the user does not have pro batch access" do + before do + AlaveteliFeatures.backend.disable_actor(:pro_batch_access, pro_user) + end + let(:pro_user) { FactoryBot.create(:pro_user) } it 'redirects them to the standard request form' do @@ -176,6 +180,10 @@ context "the user does not have pro batch access" do + before do + AlaveteliFeatures.backend.disable_actor(:pro_batch_access, pro_user) + end + let(:pro_user) { FactoryBot.create(:pro_user) } it 'redirects them to the standard request form' do diff --git a/spec/integration/alaveteli_pro/request_creation_spec.rb b/spec/integration/alaveteli_pro/request_creation_spec.rb index dd38b071eb..3930030cc9 100644 --- a/spec/integration/alaveteli_pro/request_creation_spec.rb +++ b/spec/integration/alaveteli_pro/request_creation_spec.rb @@ -13,6 +13,8 @@ end it "doesn't show the link to the batch request form to standard users" do + AlaveteliFeatures.backend.disable_actor(:pro_batch_access, pro_user) + using_pro_session(pro_user_session) do # New request form create_pro_request(public_body) From a5a9e99016794772d3965b6057e4e5e4dab82288 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Thu, 29 Aug 2019 15:39:34 +0100 Subject: [PATCH 015/124] Add rescue blocks for RecordNotUnique excpetions Add around enabling pro_batch_access as this can cause an error. Eventually these will be removed once refactoring has been completed. --- .../batch_request_authority_searches_controller_spec.rb | 5 ++++- spec/integration/alaveteli_pro/batch_request_spec.rb | 5 ++++- spec/integration/alaveteli_pro/request_creation_spec.rb | 5 ++++- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb b/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb index 84ae7af0d4..df56bbd4ea 100644 --- a/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb @@ -30,7 +30,10 @@ describe AlaveteliPro::BatchRequestAuthoritySearchesController do let(:pro_user) do user = FactoryBot.create(:pro_user) - AlaveteliFeatures.backend.enable_actor(:pro_batch_access, user) + begin + AlaveteliFeatures.backend.enable_actor(:pro_batch_access, user) + rescue ActiveRecord::RecordNotUnique + end user end diff --git a/spec/integration/alaveteli_pro/batch_request_spec.rb b/spec/integration/alaveteli_pro/batch_request_spec.rb index d818f3949b..10e66b1853 100644 --- a/spec/integration/alaveteli_pro/batch_request_spec.rb +++ b/spec/integration/alaveteli_pro/batch_request_spec.rb @@ -44,7 +44,10 @@ def search_results describe "creating batch requests in alaveteli_pro" do let(:pro_user) do user = FactoryBot.create(:pro_user) - AlaveteliFeatures.backend.enable_actor(:pro_batch_access, user) + begin + AlaveteliFeatures.backend.enable_actor(:pro_batch_access, user) + rescue ActiveRecord::RecordNotUnique + end user end diff --git a/spec/integration/alaveteli_pro/request_creation_spec.rb b/spec/integration/alaveteli_pro/request_creation_spec.rb index 3930030cc9..fc50916f65 100644 --- a/spec/integration/alaveteli_pro/request_creation_spec.rb +++ b/spec/integration/alaveteli_pro/request_creation_spec.rb @@ -23,7 +23,10 @@ end it "shows the link to the batch request form to pro batch users" do - AlaveteliFeatures.backend.enable_actor(:pro_batch_access, pro_user) + begin + AlaveteliFeatures.backend.enable_actor(:pro_batch_access, pro_user) + rescue ActiveRecord::RecordNotUnique + end using_pro_session(pro_user_session) do # New request form From 991554f39321f46725f65218a06ac9df25b7954e Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Wed, 28 Aug 2019 14:50:14 +0100 Subject: [PATCH 016/124] Refactor User#setup_pro_account This has no functional change but will make it easier to add more responsibility to this method in the future. --- app/models/user.rb | 4 ++-- spec/models/user_spec.rb | 8 ++++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index d561966625..10d364e22e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -709,8 +709,8 @@ def verify_otp_code end def setup_pro_account(role) - return unless role == Role.pro_role && feature_enabled?(:pro_pricing) - pro_account || build_pro_account + return unless role == Role.pro_role + pro_account || build_pro_account if feature_enabled?(:pro_pricing) end def update_pro_account diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 8bb1a1f103..0536563ed0 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1708,20 +1708,24 @@ def create_user(options = {}) describe 'role callbacks' do + let(:user) { FactoryBot.build(:user) } + context 'with pro pricing enabled', feature: :pro_pricing do + it 'creates pro account when pro role added' do - user = FactoryBot.build(:user) expect { user.add_role :pro }.to change(user, :pro_account). from(nil).to(ProAccount) end + end context 'without pro pricing enabled' do + it 'does not create pro account when pro role is added' do - user = FactoryBot.build(:user) expect { user.add_role :pro }.to_not change(user, :pro_account). from(nil) end + end end From 90b67cd48c28e118e113a05291a764ad58262ffd Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Thu, 15 Aug 2019 09:21:01 +0100 Subject: [PATCH 017/124] Extract granting pro access into service object --- .../alaveteli_pro/subscriptions_controller.rb | 17 ---- app/models/user.rb | 1 + app/services/alaveteli_pro/access.rb | 40 ++++++++ .../subscriptions_controller_spec.rb | 71 -------------- spec/models/user_spec.rb | 18 ++++ spec/services/alaveteli_pro/access_spec.rb | 96 +++++++++++++++++++ 6 files changed, 155 insertions(+), 88 deletions(-) create mode 100644 app/services/alaveteli_pro/access.rb create mode 100644 spec/services/alaveteli_pro/access_spec.rb diff --git a/app/controllers/alaveteli_pro/subscriptions_controller.rb b/app/controllers/alaveteli_pro/subscriptions_controller.rb index fbd5610df8..a3dcffd684 100644 --- a/app/controllers/alaveteli_pro/subscriptions_controller.rb +++ b/app/controllers/alaveteli_pro/subscriptions_controller.rb @@ -99,23 +99,6 @@ def create current_user.add_role(:pro) - # enable the mail poller only if the POP polling is configured AND it - # has not already been enabled for this user (raises an error) - if (AlaveteliConfiguration.production_mailer_retriever_method == 'pop' && - !feature_enabled?(:accept_mail_from_poller, current_user)) - AlaveteliFeatures. - backend. - enable_actor(:accept_mail_from_poller, current_user) - end - - unless feature_enabled?(:notifications, current_user) - AlaveteliFeatures.backend.enable_actor(:notifications, current_user) - end - - unless feature_enabled?(:pro_batch_access, current_user) - AlaveteliFeatures.backend.enable_actor(:pro_batch_access, current_user) - end - flash[:notice] = { :partial => "alaveteli_pro/subscriptions/signup_message.html.erb" } flash[:new_pro_user] = true diff --git a/app/models/user.rb b/app/models/user.rb index 10d364e22e..3599212dfd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -711,6 +711,7 @@ def verify_otp_code def setup_pro_account(role) return unless role == Role.pro_role pro_account || build_pro_account if feature_enabled?(:pro_pricing) + AlaveteliPro::Access.grant(self) end def update_pro_account diff --git a/app/services/alaveteli_pro/access.rb b/app/services/alaveteli_pro/access.rb new file mode 100644 index 0000000000..8b454d9125 --- /dev/null +++ b/app/services/alaveteli_pro/access.rb @@ -0,0 +1,40 @@ +## +# A service object to grant and revoke users access to Alaveteli Professional +# features +# +# Usage: +# AlaveteliPro::Access.grant(user) +# +# TODO: +# AlaveteliPro::Access.revoke(user) +# +class AlaveteliPro::Access + include AlaveteliFeatures::Helpers + + def self.grant(*args, &block) + new(*args, &block).grant + end + + attr_reader :user + + def initialize(user) + @user = user + end + + def grant + # enable the mail poller only if the POP polling is configured AND it + # has not already been enabled for this user (raises an error) + if (AlaveteliConfiguration.production_mailer_retriever_method == 'pop' && + !feature_enabled?(:accept_mail_from_poller, user)) + AlaveteliFeatures.backend.enable_actor(:accept_mail_from_poller, user) + end + + unless feature_enabled?(:notifications, user) + AlaveteliFeatures.backend.enable_actor(:notifications, user) + end + + unless feature_enabled?(:pro_batch_access, user) + AlaveteliFeatures.backend.enable_actor(:pro_batch_access, user) + end + end +end diff --git a/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb b/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb index c1313cfdf4..894799c940 100644 --- a/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb @@ -77,24 +77,6 @@ expect(user.is_pro?).to eq(true) end - it 'does not enable pop polling by default' do - result = - AlaveteliFeatures.backend[:accept_mail_from_poller].enabled?(user) - expect(result).to eq(false) - end - - it 'enables daily summary notifications for the user' do - result = - AlaveteliFeatures.backend[:notifications].enabled?(user) - expect(result).to eq(true) - end - - it 'enables batch for the user' do - result = - AlaveteliFeatures.backend[:pro_batch_access].enabled?(user) - expect(result).to eq(true) - end - it 'welcomes the new user' do partial_file = "alaveteli_pro/subscriptions/signup_message.html.erb" expect(flash[:notice]).to eq({ :partial => partial_file }) @@ -147,35 +129,6 @@ end - context 'the user previously had some pro features enabled' do - - def successful_signup - post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => 'coupon_code' - } - end - - it 'does not raise an error if the user already uses the poller' do - AlaveteliFeatures.backend.enable_actor(:accept_mail_from_poller, user) - expect { successful_signup }.not_to raise_error - end - - it 'does not raise an error if the user already has notifications' do - AlaveteliFeatures.backend.enable_actor(:notifications, user) - expect { successful_signup }.not_to raise_error - end - - it 'does not raise an error if the user already has batch' do - AlaveteliFeatures.backend.enable_actor(:pro_batch_access, user) - expect { successful_signup }.not_to raise_error - end - - end - context 'with a successful transaction' do before do post :create, params: { @@ -190,30 +143,6 @@ def successful_signup include_examples 'successful example' end - context 'when pop polling is enabled' do - - before do - allow(AlaveteliConfiguration). - to receive(:production_mailer_retriever_method). - and_return('pop') - - post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => '' - } - end - - it 'enables pop polling for the user' do - result = - AlaveteliFeatures.backend[:accept_mail_from_poller].enabled?(user) - expect(result).to eq(true) - end - - end - context 'with coupon code' do before do post :create, params: { diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0536563ed0..8a53099890 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1710,6 +1710,24 @@ def create_user(options = {}) let(:user) { FactoryBot.build(:user) } + context 'adding unknown role' do + + it 'should not call grant pro access' do + expect(AlaveteliPro::Access).to_not receive(:grant) + user.add_role(:unknown) + end + + end + + context 'adding pro role' do + + it 'should call grant pro access' do + expect(AlaveteliPro::Access).to receive(:grant).with(user) + user.add_role(:pro) + end + + end + context 'with pro pricing enabled', feature: :pro_pricing do it 'creates pro account when pro role added' do diff --git a/spec/services/alaveteli_pro/access_spec.rb b/spec/services/alaveteli_pro/access_spec.rb new file mode 100644 index 0000000000..6bd70399df --- /dev/null +++ b/spec/services/alaveteli_pro/access_spec.rb @@ -0,0 +1,96 @@ +require 'spec_helper' + +RSpec.describe AlaveteliPro::Access do + describe '.grant' do + + it 'initialise instance' do + args = [double(:arg1), double(:arg2)] + block = -> { double(:block) } + expect(described_class).to receive(:new).with(*args) do |&received_block| + expect(received_block).to eq(block) + double.as_null_object + end + described_class.grant(*args, &block) + end + + it 'returns #grant' do + result = double('return value') + instance = double(described_class, grant: result) + allow(described_class).to receive(:new).and_return(instance) + expect(described_class.grant).to eq(instance.grant) + end + + end + + describe '.new' do + + it 'assigns #user' do + user = double(:user) + instance = described_class.new(user) + expect(instance.user).to eq(user) + end + + end + + describe '#grant' do + let(:user) { FactoryBot.build(:user) } + let(:instance) { described_class.new(user) } + + def feature_enabled?(feature, user) + AlaveteliFeatures.backend[feature].enabled?(user) + end + + it 'does not enable pop polling by default' do + expect { instance.grant }.to_not change { + feature_enabled?(:accept_mail_from_poller, user) + } + end + + it 'enables daily summary notifications for the user' do + expect { instance.grant }.to change { + feature_enabled?(:notifications, user) + }.to(true) + end + + it 'enables batch for the user' do + expect { instance.grant }.to change { + feature_enabled?(:pro_batch_access, user) + }.to(true) + end + + context 'the user previously had some pro features enabled' do + + it 'does not raise an error if the user already has notifications' do + AlaveteliFeatures.backend.enable_actor(:notifications, user) + expect { instance.call }.not_to raise_error + end + + it 'does not raise an error if the user already has batch' do + AlaveteliFeatures.backend.enable_actor(:pro_batch_access, user) + expect { instance.call }.not_to raise_error + end + + end + + context 'when pop polling is enabled' do + + before do + allow(AlaveteliConfiguration). + to receive(:production_mailer_retriever_method). + and_return('pop') + end + + it 'enables pop polling for the user' do + expect { instance.grant }.to change { + feature_enabled?(:accept_mail_from_poller, user) + }.to(true) + end + + it 'does not raise an error if the user already uses the poller' do + AlaveteliFeatures.backend.enable_actor(:accept_mail_from_poller, user) + expect { instance.call }.not_to raise_error + end + + end + end +end From ffb63142ff6690740b3677d85f8920e5e1ce7c0a Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Wed, 28 Aug 2019 15:04:24 +0100 Subject: [PATCH 018/124] Refactor Alaveteli feature helper specs - Fix running specs outside of Rails - Fix possible transient errors when backend is changed - Remove unused spec setup --- .../lib/alaveteli_features.rb | 2 +- .../spec/alaveteli_features_spec.rb | 2 + .../constraints/feature_constraint_spec.rb | 5 +- .../spec/helpers/feature_enabled_spec.rb | 46 +++++++------------ 4 files changed, 23 insertions(+), 32 deletions(-) diff --git a/gems/alaveteli_features/lib/alaveteli_features.rb b/gems/alaveteli_features/lib/alaveteli_features.rb index 567acc5158..a88cc988fc 100644 --- a/gems/alaveteli_features/lib/alaveteli_features.rb +++ b/gems/alaveteli_features/lib/alaveteli_features.rb @@ -3,7 +3,7 @@ require "alaveteli_features/constraints" require "alaveteli_features/railtie" if defined?(Rails) require "flipper" -require "flipper-active_record" +require "flipper-active_record" if defined?(Rails) module AlaveteliFeatures def self.backend diff --git a/gems/alaveteli_features/spec/alaveteli_features_spec.rb b/gems/alaveteli_features/spec/alaveteli_features_spec.rb index c20de94e52..bba7016ef2 100644 --- a/gems/alaveteli_features/spec/alaveteli_features_spec.rb +++ b/gems/alaveteli_features/spec/alaveteli_features_spec.rb @@ -12,7 +12,9 @@ it 'should allow you to set the backend' do test_backend = Flipper.new(Flipper::Adapters::Memory.new) + old_backend = AlaveteliFeatures.backend AlaveteliFeatures.backend = test_backend expect(AlaveteliFeatures.backend).to be test_backend + AlaveteliFeatures.backend = old_backend end end diff --git a/gems/alaveteli_features/spec/constraints/feature_constraint_spec.rb b/gems/alaveteli_features/spec/constraints/feature_constraint_spec.rb index a0bba6b5f7..9344b5fcaa 100644 --- a/gems/alaveteli_features/spec/constraints/feature_constraint_spec.rb +++ b/gems/alaveteli_features/spec/constraints/feature_constraint_spec.rb @@ -3,8 +3,11 @@ describe AlaveteliFeatures::Constraints::FeatureConstraint do let(:test_backend) { Flipper.new(Flipper::Adapters::Memory.new) } - before do + around do |example| + old_backend = AlaveteliFeatures.backend AlaveteliFeatures.backend = test_backend + example.call + AlaveteliFeatures.backend = old_backend end describe "#matches?" do diff --git a/gems/alaveteli_features/spec/helpers/feature_enabled_spec.rb b/gems/alaveteli_features/spec/helpers/feature_enabled_spec.rb index b40a15a7fb..349e91de49 100644 --- a/gems/alaveteli_features/spec/helpers/feature_enabled_spec.rb +++ b/gems/alaveteli_features/spec/helpers/feature_enabled_spec.rb @@ -5,35 +5,24 @@ describe AlaveteliFeatures::Helpers do let(:instance) { Class.new { include AlaveteliFeatures::Helpers }.new } let(:test_backend) { Flipper.new(Flipper::Adapters::Memory.new) } - let(:user_class) do - # A test class to let us test the actor-based feature flipping - class User - attr_reader :id - - def initialize(id, admin) - @id = id - @admin = admin - end - - def admin? - @admin - end - - # Must respond to flipper_id - alias_method :flipper_id, :id + + # A test class to let us test the actor-based feature flipping + class MockUser + attr_reader :id + + def initialize(id) + @id = id end + + # Must respond to flipper_id + alias flipper_id id end - before do + around do |example| + old_backend = AlaveteliFeatures.backend AlaveteliFeatures.backend = test_backend - # Seems to be the only way to make sure we don't register a group twice - begin - AlaveteliFeatures.backend.group(:admins) - rescue Flipper::GroupNotRegistered - Flipper.register :admins do |actor| - actor.respond_to?(:admin?) && actor.admin? - end - end + example.call + AlaveteliFeatures.backend = old_backend end describe "#feature_enabled?" do @@ -48,12 +37,9 @@ def admin? end it "should pass on other arguments to the backend" do - user1 = user_class.new(1, true) - - mock_backend = double("backend") - AlaveteliFeatures.backend = mock_backend + user1 = MockUser.new(1) - expect(mock_backend).to( + expect(AlaveteliFeatures.backend).to( receive(:enabled?).with(:test_feature, user1) ) instance.feature_enabled?(:test_feature, user1) From 400f093ab4c4b0a05eb1863d8de433b2cfbfb351 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Wed, 28 Aug 2019 15:04:24 +0100 Subject: [PATCH 019/124] Add Alaveteli feature helper to enable actors Refactor existing uses of `AlaveteliFeatures.backend.enable_actor` to use the new helper method. --- app/services/alaveteli_pro/access.rb | 17 ++---- .../lib/alaveteli_features/helpers.rb | 6 +++ .../spec/helpers/enable_actor_spec.rb | 52 +++++++++++++++++++ spec/services/alaveteli_pro/access_spec.rb | 19 ------- 4 files changed, 63 insertions(+), 31 deletions(-) create mode 100644 gems/alaveteli_features/spec/helpers/enable_actor_spec.rb diff --git a/app/services/alaveteli_pro/access.rb b/app/services/alaveteli_pro/access.rb index 8b454d9125..5bab598c3e 100644 --- a/app/services/alaveteli_pro/access.rb +++ b/app/services/alaveteli_pro/access.rb @@ -22,19 +22,12 @@ def initialize(user) end def grant - # enable the mail poller only if the POP polling is configured AND it - # has not already been enabled for this user (raises an error) - if (AlaveteliConfiguration.production_mailer_retriever_method == 'pop' && - !feature_enabled?(:accept_mail_from_poller, user)) - AlaveteliFeatures.backend.enable_actor(:accept_mail_from_poller, user) + # enable the mail poller only if the POP polling is configured + if AlaveteliConfiguration.production_mailer_retriever_method == 'pop' + enable_actor(:accept_mail_from_poller, user) end - unless feature_enabled?(:notifications, user) - AlaveteliFeatures.backend.enable_actor(:notifications, user) - end - - unless feature_enabled?(:pro_batch_access, user) - AlaveteliFeatures.backend.enable_actor(:pro_batch_access, user) - end + enable_actor(:notifications, user) + enable_actor(:pro_batch_access, user) end end diff --git a/gems/alaveteli_features/lib/alaveteli_features/helpers.rb b/gems/alaveteli_features/lib/alaveteli_features/helpers.rb index cd12a8d6c8..919a2994cd 100644 --- a/gems/alaveteli_features/lib/alaveteli_features/helpers.rb +++ b/gems/alaveteli_features/lib/alaveteli_features/helpers.rb @@ -3,5 +3,11 @@ module Helpers def feature_enabled?(feature, *args) AlaveteliFeatures.backend.enabled?(feature, *args) end + + def enable_actor(feature, user) + # check feature hasn't already been enabled for the user + return true if feature_enabled?(feature, user) + AlaveteliFeatures.backend.enable_actor(feature, user) + end end end diff --git a/gems/alaveteli_features/spec/helpers/enable_actor_spec.rb b/gems/alaveteli_features/spec/helpers/enable_actor_spec.rb new file mode 100644 index 0000000000..88154b43fc --- /dev/null +++ b/gems/alaveteli_features/spec/helpers/enable_actor_spec.rb @@ -0,0 +1,52 @@ +require 'spec_helper' +require 'flipper/adapters/memory' +require 'alaveteli_features/helpers' + +describe AlaveteliFeatures::Helpers do + let(:instance) { Class.new { include AlaveteliFeatures::Helpers }.new } + let(:test_backend) { Flipper.new(Flipper::Adapters::Memory.new) } + + # A test class to let us test the actor-based feature flipping + class MockUser + attr_reader :id + + def initialize(id) + @id = id + end + + # Must respond to flipper_id + alias flipper_id id + end + + around do |example| + old_backend = AlaveteliFeatures.backend + AlaveteliFeatures.backend = test_backend + example.call + AlaveteliFeatures.backend = old_backend + end + + describe '#enable_actor' do + let(:user) { MockUser.new(1) } + + it 'should respond true when an actor already enabled' do + AlaveteliFeatures.backend.enable_actor(:test_feature, user) + expect(instance.enable_actor(:test_feature, user)).to eq(true) + end + + it 'should respond true when an actor is enabled' do + expect(instance.enable_actor(:test_feature, user)).to eq(true) + end + + context 'persisted backend' do + let(:test_backend) do + skip('Rails and database connection required') unless defined?(Rails) + Flipper.new(Flipper::Adapters::ActiveRecord.new) + end + + it 'does not raise PG::UniqueViolation if actor is already enabled' do + AlaveteliFeatures.backend.enable_actor(:test_feature, user) + expect { instance.enable_actor(:test_feature, user) }.not_to raise_error + end + end + end +end diff --git a/spec/services/alaveteli_pro/access_spec.rb b/spec/services/alaveteli_pro/access_spec.rb index 6bd70399df..b9f76dfcf2 100644 --- a/spec/services/alaveteli_pro/access_spec.rb +++ b/spec/services/alaveteli_pro/access_spec.rb @@ -58,20 +58,6 @@ def feature_enabled?(feature, user) }.to(true) end - context 'the user previously had some pro features enabled' do - - it 'does not raise an error if the user already has notifications' do - AlaveteliFeatures.backend.enable_actor(:notifications, user) - expect { instance.call }.not_to raise_error - end - - it 'does not raise an error if the user already has batch' do - AlaveteliFeatures.backend.enable_actor(:pro_batch_access, user) - expect { instance.call }.not_to raise_error - end - - end - context 'when pop polling is enabled' do before do @@ -86,11 +72,6 @@ def feature_enabled?(feature, user) }.to(true) end - it 'does not raise an error if the user already uses the poller' do - AlaveteliFeatures.backend.enable_actor(:accept_mail_from_poller, user) - expect { instance.call }.not_to raise_error - end - end end end From a5b1fb4bb6b0679cef0be3235dae7fd07e010a35 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Thu, 29 Aug 2019 15:40:32 +0100 Subject: [PATCH 020/124] Remove checks for RecordNotUnique We don't need these around enabled pro_batch_access now as this is done when the pro_user factory is created. --- ...batch_request_authority_searches_controller_spec.rb | 9 +-------- spec/integration/alaveteli_pro/batch_request_spec.rb | 10 +--------- .../integration/alaveteli_pro/request_creation_spec.rb | 5 ----- 3 files changed, 2 insertions(+), 22 deletions(-) diff --git a/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb b/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb index df56bbd4ea..7909c89c73 100644 --- a/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/batch_request_authority_searches_controller_spec.rb @@ -28,14 +28,7 @@ end describe AlaveteliPro::BatchRequestAuthoritySearchesController do - let(:pro_user) do - user = FactoryBot.create(:pro_user) - begin - AlaveteliFeatures.backend.enable_actor(:pro_batch_access, user) - rescue ActiveRecord::RecordNotUnique - end - user - end + let(:pro_user) { FactoryBot.create(:pro_user) } describe "#index" do let(:authority_1) { FactoryBot.build(:public_body) } diff --git a/spec/integration/alaveteli_pro/batch_request_spec.rb b/spec/integration/alaveteli_pro/batch_request_spec.rb index 10e66b1853..94095b6fa7 100644 --- a/spec/integration/alaveteli_pro/batch_request_spec.rb +++ b/spec/integration/alaveteli_pro/batch_request_spec.rb @@ -42,15 +42,7 @@ def search_results end describe "creating batch requests in alaveteli_pro" do - let(:pro_user) do - user = FactoryBot.create(:pro_user) - begin - AlaveteliFeatures.backend.enable_actor(:pro_batch_access, user) - rescue ActiveRecord::RecordNotUnique - end - user - end - + let(:pro_user) { FactoryBot.create(:pro_user) } let!(:pro_user_session) { login(pro_user) } let!(:authorities) { FactoryBot.create_list(:public_body, 26) } diff --git a/spec/integration/alaveteli_pro/request_creation_spec.rb b/spec/integration/alaveteli_pro/request_creation_spec.rb index fc50916f65..9e4d967380 100644 --- a/spec/integration/alaveteli_pro/request_creation_spec.rb +++ b/spec/integration/alaveteli_pro/request_creation_spec.rb @@ -23,11 +23,6 @@ end it "shows the link to the batch request form to pro batch users" do - begin - AlaveteliFeatures.backend.enable_actor(:pro_batch_access, pro_user) - rescue ActiveRecord::RecordNotUnique - end - using_pro_session(pro_user_session) do # New request form create_pro_request(public_body) From 38341b0e8a98a96d5eb982f6ec3433ed6c716e30 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Wed, 28 Aug 2019 16:41:16 +0100 Subject: [PATCH 021/124] Refactor pro_account spec setup --- spec/models/pro_account_spec.rb | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/spec/models/pro_account_spec.rb b/spec/models/pro_account_spec.rb index fd34432d7a..435bc9c168 100644 --- a/spec/models/pro_account_spec.rb +++ b/spec/models/pro_account_spec.rb @@ -16,22 +16,13 @@ describe ProAccount, feature: :pro_pricing do - before do - StripeMock.start - end - - after do - StripeMock.stop - end + around { |example| StripeMock.mock(&example) } let(:stripe_helper) { StripeMock.create_test_helper } let(:plan) { stripe_helper.create_plan(id: 'pro', amount: 1000) } let(:customer) do - Stripe::Customer.create( - email: FactoryBot.build(:user).email, - source: stripe_helper.generate_card_token - ) + Stripe::Customer.create(source: stripe_helper.generate_card_token) end let(:subscription) do @@ -40,8 +31,13 @@ describe 'validations' do + let(:user) { FactoryBot.build(:pro_user) } + subject(:pro_account) { FactoryBot.build(:pro_account, user: user) } + + it { is_expected.to be_valid } + it 'requires a user' do - pro_account = FactoryBot.build(:pro_account, user: nil) + pro_account.user = nil expect(pro_account).not_to be_valid end @@ -76,7 +72,7 @@ context 'with invalid Stripe customer ID' do let(:pro_account) do - FactoryBot.create(:pro_account, stripe_customer_id: 'invalid_id') + FactoryBot.build(:pro_account, stripe_customer_id: 'invalid_id') end it 'raises an error' do @@ -88,7 +84,7 @@ context 'with valid Stripe customer ID' do let(:pro_account) do - FactoryBot.create(:pro_account, stripe_customer_id: customer.id) + FactoryBot.build(:pro_account, stripe_customer_id: customer.id) end it 'finds the Stripe::Customer linked to the ProAccount' do From 5d583abce3b7aa9268bd20f772e58b96cbdb3f05 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Wed, 28 Aug 2019 16:41:16 +0100 Subject: [PATCH 022/124] Refactor pro account creation This simplify creating Stripe customer and keeping their email addresses in sync with local users by calling #update_stripe_customer. In theory this could be done automatically as an after_commit callback but this would break other specs. --- app/models/pro_account.rb | 22 +++--- app/models/user.rb | 3 +- .../subscriptions_controller_spec.rb | 4 +- spec/models/pro_account_spec.rb | 67 ++++++++++++------- spec/models/user_spec.rb | 6 +- 5 files changed, 58 insertions(+), 44 deletions(-) diff --git a/app/models/pro_account.rb b/app/models/pro_account.rb index ea7aeb1513..4e2f0824bf 100644 --- a/app/models/pro_account.rb +++ b/app/models/pro_account.rb @@ -19,8 +19,6 @@ class ProAccount < ApplicationRecord validates :user, presence: true - before_create :set_stripe_customer_id - def active? stripe_customer.present? && stripe_customer.subscriptions.any? end @@ -29,20 +27,22 @@ def stripe_customer @stripe_customer ||= stripe_customer! end - def update_email_address - return unless stripe_customer - stripe_customer.email = user.email + def update_stripe_customer + return unless feature_enabled?(:pro_pricing) + + @stripe_customer = stripe_customer || Stripe::Customer.new + + update_email stripe_customer.save + update(stripe_customer_id: stripe_customer.id) end private - def set_stripe_customer_id - return unless feature_enabled? :pro_pricing - self.stripe_customer_id ||= begin - @stripe_customer = Stripe::Customer.create(email: user.email) - stripe_customer.id - end + def update_email + return unless stripe_customer.try(:email) != user.email + + stripe_customer.email = user.email end def stripe_customer! diff --git a/app/models/user.rb b/app/models/user.rb index 3599212dfd..f86c504344 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -715,8 +715,7 @@ def setup_pro_account(role) end def update_pro_account - return unless is_pro? && pro_account - pro_account.update_email_address if email_changed? + pro_account.update_stripe_customer if pro_account end end diff --git a/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb b/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb index 894799c940..f8b67f8bfc 100644 --- a/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb @@ -611,9 +611,7 @@ context 'user has no Stripe id' do let(:user) do - user = FactoryBot.create(:pro_user) - user.pro_account.update(stripe_customer_id: nil) - user + FactoryBot.create(:pro_user) end before do diff --git a/spec/models/pro_account_spec.rb b/spec/models/pro_account_spec.rb index 435bc9c168..3a7d06a295 100644 --- a/spec/models/pro_account_spec.rb +++ b/spec/models/pro_account_spec.rb @@ -43,25 +43,59 @@ end - describe 'create callbacks' do + describe '#update_stripe_customer', feature: :pro_pricing do - it 'creates Stripe customer and stores Stripe customer ID' do - pro_account = FactoryBot.build(:pro_account, stripe_customer_id: nil) - expect(Stripe::Customer).to receive(:create).and_call_original - pro_account.run_callbacks :create - expect(pro_account.stripe_customer_id).to_not be_nil + let(:user) { FactoryBot.build(:pro_user) } + let(:pro_account) { FactoryBot.build(:pro_account, user: user) } + + before do + allow(Stripe::Customer).to receive(:new).and_return(customer) + end + + it 'stores Stripe customer ID' do + expect { + pro_account.update_stripe_customer + }.to change(pro_account, :stripe_customer_id).from(nil) + end + + it 'creates Stripe customer' do + allow(customer).to receive(:save) + pro_account.update_stripe_customer + expect(customer).to have_received(:save) + end + + it 'sets Stripe customer email' do + pro_account.update_stripe_customer + expect(customer.email).to eq user.email end context 'with pro_pricing disabled' do + it 'does not store Stripe customer ID' do + with_feature_disabled(:pro_pricing) do + expect { + pro_account.update_stripe_customer + }.to_not change(pro_account, :stripe_customer_id) + end + end + it 'does not create a Stripe customer' do - with_feature_disabled(:alaveteli_pro) do - pro_account = FactoryBot.build(:pro_account, stripe_customer_id: nil) - pro_account.run_callbacks :create + with_feature_disabled(:pro_pricing) do + allow(customer).to receive(:save) + pro_account.update_stripe_customer + expect(customer).to_not have_received(:save) expect(pro_account.stripe_customer_id).to be_nil end end + it 'does not set Stripe customer email' do + with_feature_disabled(:pro_pricing) do + allow(customer).to receive(:email=) + pro_account.update_stripe_customer + expect(customer).to_not have_received(:email=) + end + end + end end @@ -128,19 +162,4 @@ end - describe '#update_email_address' do - let(:user) { FactoryBot.build(:user, email: 'bilbo@example.com') } - let(:pro_account) { FactoryBot.create(:pro_account, user: user) } - - before { allow(pro_account).to receive(:stripe_customer) { customer } } - - it 'update Stripe customer email address' do - expect(customer.email).to_not eq user.email - expect(customer).to receive(:save) - pro_account.update_email_address - expect(customer.email).to eq user.email - end - - end - end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 8a53099890..32d235440a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1756,12 +1756,10 @@ def create_user(options = {}) before do allow(user).to receive(:pro_account).and_return(pro_account) - allow(user).to receive(:is_pro?).and_return(true) - allow(user).to receive(:email_changed?).and_return(true) end - it 'calls update_email_address on Pro Account' do - expect(pro_account).to receive(:update_email_address) + it 'calls update_stripe_customer on Pro Account' do + expect(pro_account).to receive(:update_stripe_customer) user.run_callbacks :update end From d362d4be0a2840e6877316338abb96c71454c962 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Wed, 4 Sep 2019 11:33:34 +0100 Subject: [PATCH 023/124] Update stripe-ruby-mock gem to patched version This includes fixes for mock list objects pagination. For example customer subscriptions. --- Gemfile | 3 ++- Gemfile.lock | 16 +++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/Gemfile b/Gemfile index ad5d7ea42c..ce84b21249 100644 --- a/Gemfile +++ b/Gemfile @@ -166,7 +166,8 @@ group :test do gem 'coveralls', '~> 0.8.0', :require => false gem 'capybara', '~> 3.5.0' gem 'delorean', '~> 2.1.0' - gem 'stripe-ruby-mock', ['~> 2.5.4', '< 2.5.7'] + gem 'stripe-ruby-mock', git: 'https://github.com/gbp/stripe-ruby-mock', + branch: 'develop' gem('rails-controller-testing') end diff --git a/Gemfile.lock b/Gemfile.lock index 8644ab0e5a..32cc76c343 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,13 @@ +GIT + remote: https://github.com/gbp/stripe-ruby-mock + revision: 116613f5ce1757bca202d31d815cb58377f720d4 + branch: develop + specs: + stripe-ruby-mock (2.5.6) + dante (>= 0.2.0) + multi_json (~> 1.0) + stripe (>= 2.0.3) + GIT remote: https://github.com/heapsource/active_model_otp.git revision: 55d93a3979907d8382c83c9b0f3e94e3186167fc @@ -374,10 +384,6 @@ GEM statistics2 (0.54) stripe (3.29.0) faraday (~> 0.10) - stripe-ruby-mock (2.5.6) - dante (>= 0.2.0) - multi_json (~> 1.0) - stripe (>= 2.0.3) syslog_protocol (0.9.2) term-ansicolor (1.6.0) tins (~> 1.0) @@ -498,7 +504,7 @@ DEPENDENCIES statistics2 (~> 0.54) strip_attributes! stripe (~> 3.29.0) - stripe-ruby-mock (~> 2.5.4, < 2.5.7) + stripe-ruby-mock! syslog_protocol (~> 0.9.0) therubyracer (~> 0.12.0) thin (~> 1.5.0, < 1.6.0) From 44ad32c5d942c2c12853af44a07731aa5b550cd0 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Tue, 20 Aug 2019 16:34:24 +0100 Subject: [PATCH 024/124] Add AlaveteliPro::Subscription class Better interface for querying a customers subscriptions. --- app/models/alaveteli_pro/subscription.rb | 21 +++ .../alaveteli_pro/subscription_collection.rb | 55 ++++++++ app/models/pro_account.rb | 8 +- .../subscription_collection_spec.rb | 129 ++++++++++++++++++ .../models/alaveteli_pro/subscription_spec.rb | 34 +++++ 5 files changed, 246 insertions(+), 1 deletion(-) create mode 100644 app/models/alaveteli_pro/subscription.rb create mode 100644 app/models/alaveteli_pro/subscription_collection.rb create mode 100644 spec/models/alaveteli_pro/subscription_collection_spec.rb create mode 100644 spec/models/alaveteli_pro/subscription_spec.rb diff --git a/app/models/alaveteli_pro/subscription.rb b/app/models/alaveteli_pro/subscription.rb new file mode 100644 index 0000000000..bf4357f31d --- /dev/null +++ b/app/models/alaveteli_pro/subscription.rb @@ -0,0 +1,21 @@ +module AlaveteliPro + ## + # This class adds wraps a Stripe::Subscription object to customise behaviour + # and to add useful helper methods. + # + class Subscription < SimpleDelegator + # state + def active? + status == 'active' + end + + private + + def method_missing(*args) + # Forward missing methods such as #coupon= as on a blank subscription + # this wouldn't be delegated due to how Stripe::APIResource instances + # use meta programming to dynamically define setting methods. + __getobj__.public_send(*args) + end + end +end diff --git a/app/models/alaveteli_pro/subscription_collection.rb b/app/models/alaveteli_pro/subscription_collection.rb new file mode 100644 index 0000000000..e345601bc2 --- /dev/null +++ b/app/models/alaveteli_pro/subscription_collection.rb @@ -0,0 +1,55 @@ +module AlaveteliPro + ## + # This class is responsible for loading and wrapping Stripe subscriptions as + # AlaveteliPro::Subscription objects. This allows us to easily customise + # behaviour and add helper methods. + # + class SubscriptionCollection + include Enumerable + + def self.for_customer(customer) + new(customer) + end + + def initialize(customer) + @customer = customer + end + + def build + AlaveteliPro::Subscription.new( + Stripe::Subscription.new.tap do |subscription| + subscription.update_attributes(customer: @customer) + end + ) + end + + # scope + def active + select(&:active?) + end + + # enumerable + def each(&block) + if block_given? + wrapped_block = -> (subscription) do + block.call(AlaveteliPro::Subscription.new(subscription)) + end + + if subscriptions.is_a?(Stripe::ListObject) + subscriptions.auto_paging_each(&wrapped_block) + else + subscriptions.each(&wrapped_block) + end + else + to_enum(:each) + end + end + + private + + def subscriptions + return [] unless @customer + @customer.subscriptions + end + end +end diff --git a/app/models/pro_account.rb b/app/models/pro_account.rb index 4e2f0824bf..2321e1b5e8 100644 --- a/app/models/pro_account.rb +++ b/app/models/pro_account.rb @@ -20,7 +20,13 @@ class ProAccount < ApplicationRecord validates :user, presence: true def active? - stripe_customer.present? && stripe_customer.subscriptions.any? + subscriptions.active.any? + end + + def subscriptions + @subscriptions ||= AlaveteliPro::SubscriptionCollection.for_customer( + stripe_customer + ) end def stripe_customer diff --git a/spec/models/alaveteli_pro/subscription_collection_spec.rb b/spec/models/alaveteli_pro/subscription_collection_spec.rb new file mode 100644 index 0000000000..c292ad88d0 --- /dev/null +++ b/spec/models/alaveteli_pro/subscription_collection_spec.rb @@ -0,0 +1,129 @@ +require 'spec_helper' + +RSpec.describe AlaveteliPro::SubscriptionCollection do + + let(:collection) { described_class.new(customer) } + let(:customer) { double(:customer) } + + let(:active_subscription) { double(:subscription, status: 'active') } + let(:incomplete_subscription) { double(:subscription, status: 'incomplete') } + + describe '.for_customer' do + + it 'should return instance for customer' do + collection = described_class.for_customer(customer) + expect(collection).to be_a described_class + end + + it 'should pass customer to instance' do + expect(described_class).to receive(:new).with(customer) + described_class.for_customer(customer) + end + + end + + describe '.new' do + + it 'should store customer instance variable' do + expect(collection.instance_variable_get(:@customer)).to eq customer + end + + end + + describe '#build' do + + let(:collection) { described_class.new(customer) } + let(:subscription) { collection.build } + + it 'should build new subscription' do + expect(subscription).to be_a AlaveteliPro::Subscription + end + + it 'should delegated to a Stripe subscription' do + expect(subscription.__getobj__).to be_a Stripe::Subscription + end + + it 'should set customer object' do + expect(subscription.customer).to eq customer + end + + end + + describe '#active' do + + before do + allow(customer).to receive(:subscriptions).and_return( + [active_subscription, incomplete_subscription] + ) + end + + it 'should return any active subscription' do + expect(collection.active).to match_array [active_subscription] + end + + end + + describe '#each' do + + context 'without customer' do + + let(:customer) { nil } + + it 'returns no subscriptions' do + expect(collection.count).to eq 0 + end + + end + + context 'with Stripe subscriptions' do + + let(:subscriptions) do + Stripe::ListObject.new + end + + before do + allow(customer).to receive(:subscriptions).and_return(subscriptions) + allow(subscriptions).to receive(:auto_paging_each). + and_yield(active_subscription). + and_yield(incomplete_subscription) + end + + it 'returns to correct amount of objects' do + expect(collection.count).to eq 2 + end + + it 'wraps subscriptions as AlaveteliPro::Subscription objects' do + expect(collection.to_a).to all(be_a AlaveteliPro::Subscription) + end + + end + + context 'without Stripe subscriptions' do + + before do + allow(customer).to receive(:subscriptions).and_return( + [active_subscription, incomplete_subscription, active_subscription] + ) + end + + it 'returns to correct amount of objects' do + expect(collection.count).to eq 3 + end + + it 'wraps subscriptions as AlaveteliPro::Subscription objects' do + expect(collection.to_a).to all(be_a AlaveteliPro::Subscription) + end + + end + + context 'without block' do + + it 'should return a Enumerator' do + expect(collection.each).to be_a Enumerator + end + + end + + end + +end diff --git a/spec/models/alaveteli_pro/subscription_spec.rb b/spec/models/alaveteli_pro/subscription_spec.rb new file mode 100644 index 0000000000..d3fb073d6c --- /dev/null +++ b/spec/models/alaveteli_pro/subscription_spec.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +RSpec.describe AlaveteliPro::Subscription do + + let(:object) { Stripe::Subscription.new } + let(:subscription) { described_class.new(object) } + + describe '#active?' do + + it 'should return true if status is active' do + object.status = 'active' + expect(subscription.active?).to eq true + end + + it 'should return false if status is not active' do + object.status = 'other' + expect(subscription.active?).to eq false + end + + end + + describe 'missing methods' do + + it 'should delegate methods to object' do + mock_coupon = double(:coupon) + expect { subscription.coupon }.to raise_error(NoMethodError) + expect { subscription.coupon = mock_coupon }.to_not raise_error + expect(subscription.coupon).to eq mock_coupon + expect(subscription.__getobj__.coupon).to eq mock_coupon + end + + end + +end From 82c38ccddd224e2d816d4329df3fc40b326cac0c Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 16 Aug 2019 01:11:44 +0100 Subject: [PATCH 025/124] Add writable source pro account method This adds new card payment sources to Stripe customers. --- app/models/pro_account.rb | 10 ++++++++++ spec/models/pro_account_spec.rb | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/app/models/pro_account.rb b/app/models/pro_account.rb index 2321e1b5e8..fede6fbc2a 100644 --- a/app/models/pro_account.rb +++ b/app/models/pro_account.rb @@ -14,6 +14,8 @@ class ProAccount < ApplicationRecord include AlaveteliFeatures::Helpers + attr_writer :source + belongs_to :user, :inverse_of => :pro_account @@ -39,6 +41,8 @@ def update_stripe_customer @stripe_customer = stripe_customer || Stripe::Customer.new update_email + update_source + stripe_customer.save update(stripe_customer_id: stripe_customer.id) end @@ -51,6 +55,12 @@ def update_email stripe_customer.email = user.email end + def update_source + return unless @source + + stripe_customer.source = @source + end + def stripe_customer! Stripe::Customer.retrieve(stripe_customer_id) if stripe_customer_id end diff --git a/spec/models/pro_account_spec.rb b/spec/models/pro_account_spec.rb index 3a7d06a295..ed7a0da992 100644 --- a/spec/models/pro_account_spec.rb +++ b/spec/models/pro_account_spec.rb @@ -69,6 +69,15 @@ expect(customer.email).to eq user.email end + it 'sets Stripe customer default source' do + old_source = customer.default_source + pro_account.source = stripe_helper.generate_card_token + + expect { pro_account.update_stripe_customer }.to change( + customer, :default_source + ).from(old_source) + end + context 'with pro_pricing disabled' do it 'does not store Stripe customer ID' do @@ -96,6 +105,15 @@ end end + it 'does not set new Stripe customer default source' do + with_feature_disabled(:alaveteli_pro) do + pro_account.source = stripe_helper.generate_card_token + expect { pro_account.update_stripe_customer }.to_not change( + customer, :default_source + ) + end + end + end end From 11ca1d7ffbebbbd5fc8fd49da77fae89172dbbc7 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Thu, 15 Aug 2019 09:19:16 +0100 Subject: [PATCH 026/124] Refactor subscription controller --- .../alaveteli_pro/subscriptions_controller.rb | 32 ++++++------------- .../subscriptions_controller_spec.rb | 15 ++++----- 2 files changed, 17 insertions(+), 30 deletions(-) diff --git a/app/controllers/alaveteli_pro/subscriptions_controller.rb b/app/controllers/alaveteli_pro/subscriptions_controller.rb index a3dcffd684..f1eb7adf49 100644 --- a/app/controllers/alaveteli_pro/subscriptions_controller.rb +++ b/app/controllers/alaveteli_pro/subscriptions_controller.rb @@ -37,31 +37,19 @@ def create begin @token = Stripe::Token.retrieve(params[:stripeToken]) - customer = current_user.pro_account.try(:stripe_customer) + @pro_account = current_user.pro_account ||= current_user.build_pro_account + @pro_account.source = @token.id + @pro_account.update_stripe_customer - @customer = - if customer - customer.source = @token.id - customer.save - customer - else - customer = - Stripe::Customer.create(email: params[:stripeEmail], - source: @token) - - current_user.create_pro_account(stripe_customer_id: customer.id) - customer - end - - subscription_attributes = { - customer: @customer, - plan: params[:plan_id], - tax_percent: 20.0 - } + @subscription = @pro_account.subscriptions.build + @subscription.update_attributes( + plan: params.require(:plan_id), + tax_percent: 20.0, + ) - subscription_attributes[:coupon] = coupon_code if coupon_code? + @subscription.coupon = coupon_code if coupon_code? - @subscription = Stripe::Subscription.create(subscription_attributes) + @subscription.save rescue Stripe::CardError => e flash[:error] = e.message diff --git a/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb b/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb index f8b67f8bfc..33372aef5d 100644 --- a/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb @@ -53,15 +53,14 @@ end it 'creates a new Stripe customer' do - expect(assigns(:customer).email).to eq(user.email) + expect(assigns(:pro_account).stripe_customer.email). + to eq(user.email) end it 'subscribes the user to the plan' do - expected = { user: assigns(:customer).id, - plan: 'pro' } - actual = { user: assigns(:subscription).customer, - plan: assigns(:subscription).plan.id } - expect(actual).to eq(expected) + expect(assigns(:subscription).plan.id).to eq('pro') + expect(assigns(:pro_account).stripe_customer_id). + to eq(assigns(:subscription).customer) end it 'creates a pro account for the user' do @@ -70,7 +69,7 @@ it 'stores the stripe_customer_id in the pro_account' do expect(user.pro_account.stripe_customer_id). - to eq(assigns(:customer).id) + to eq(assigns(:pro_account).stripe_customer_id) end it 'adds the pro role' do @@ -208,7 +207,7 @@ end it 'updates the source from the new token' do - expect(assigns[:customer].default_source). + expect(assigns[:pro_account].stripe_customer.default_source). to eq(assigns[:token].card.id) end end From a24a2578422a77a6569362bbdc6a14d8b490c7b1 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 6 Sep 2019 16:30:02 +0100 Subject: [PATCH 027/124] Refactor payment intent controller --- .../alaveteli_pro/payment_methods_controller.rb | 8 ++++---- .../alaveteli_pro/payment_methods_controller_spec.rb | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/controllers/alaveteli_pro/payment_methods_controller.rb b/app/controllers/alaveteli_pro/payment_methods_controller.rb index 6f2d83467c..0c3d925812 100644 --- a/app/controllers/alaveteli_pro/payment_methods_controller.rb +++ b/app/controllers/alaveteli_pro/payment_methods_controller.rb @@ -6,10 +6,10 @@ def update begin @token = Stripe::Token.retrieve(params[:stripeToken]) @old_card_id = params[:old_card_id] - @customer = Stripe::Customer. - retrieve(current_user.pro_account.stripe_customer_id) - @customer.source = @token.id - @customer.save + + @pro_account = current_user.pro_account ||= current_user.build_pro_account + @pro_account.source = @token.id + @pro_account.update_stripe_customer flash[:notice] = _('Your payment details have been updated') diff --git a/spec/controllers/alaveteli_pro/payment_methods_controller_spec.rb b/spec/controllers/alaveteli_pro/payment_methods_controller_spec.rb index e1b927ba84..9936610c50 100644 --- a/spec/controllers/alaveteli_pro/payment_methods_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/payment_methods_controller_spec.rb @@ -63,12 +63,12 @@ expect(assigns(:old_card_id)).to eq(old_card_id) end - it 'retrieves the correct Stripe customer' do + it 'retrieves the correct pro account' do post :update, params: { 'stripeToken' => new_token, 'old_card_id' => old_card_id } - expect(assigns(:customer).id). + expect(assigns(:pro_account).stripe_customer_id). to eq(user.pro_account.stripe_customer_id) end From 5780f0251d94438f131a8c38aa0020feefa618af Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Tue, 20 Aug 2019 16:06:36 +0100 Subject: [PATCH 028/124] Add callback to ensure plan ID is present Use before action callback to ensure the plan ID parameter is present before processing the create subscription action. --- .../alaveteli_pro/subscriptions_controller.rb | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/app/controllers/alaveteli_pro/subscriptions_controller.rb b/app/controllers/alaveteli_pro/subscriptions_controller.rb index f1eb7adf49..9cc51f6ea8 100644 --- a/app/controllers/alaveteli_pro/subscriptions_controller.rb +++ b/app/controllers/alaveteli_pro/subscriptions_controller.rb @@ -4,6 +4,7 @@ class AlaveteliPro::SubscriptionsController < AlaveteliPro::BaseController skip_before_action :pro_user_authenticated?, only: [:create] before_action :authenticate, :prevent_duplicate_submission, only: [:create] + before_action :check_plan_exists, only: [:create] before_action :check_active_subscription, only: [:index] def index @@ -32,7 +33,8 @@ def index # "stripeTokenType"=>"card", # "stripeEmail"=>"bob@example.com", # "controller"=>"alaveteli_pro/subscriptions", - # "action"=>"create"} + # "action"=>"create", + # "plan_id"=>"WDTK-pro"} def create begin @token = Stripe::Token.retrieve(params[:stripeToken]) @@ -77,11 +79,7 @@ def create 'have not been charged. Please try again later.') end - if params[:plan_id] - redirect_to plan_path(non_namespaced_plan_id) - else - redirect_to pro_plans_path - end + redirect_to plan_path(non_namespaced_plan_id) return end @@ -145,7 +143,7 @@ def check_active_subscription end def non_namespaced_plan_id - remove_stripe_namespace(params[:plan_id]) + remove_stripe_namespace(params[:plan_id]) if params[:plan_id] end def coupon_code? @@ -169,6 +167,10 @@ def referral_coupon end end + def check_plan_exists + redirect_to(pro_plans_path) unless non_namespaced_plan_id + end + def prevent_duplicate_submission # TODO: This doesn't take the plan in to account if @user.pro_account.try(:active?) From fc817f5120686c3048a6c981af228f3b5d5405e6 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Tue, 20 Aug 2019 16:08:01 +0100 Subject: [PATCH 029/124] Refactor error redirects Both rescues are redirecting the the same path so we can extract. --- app/controllers/alaveteli_pro/subscriptions_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/alaveteli_pro/subscriptions_controller.rb b/app/controllers/alaveteli_pro/subscriptions_controller.rb index 9cc51f6ea8..49f093a682 100644 --- a/app/controllers/alaveteli_pro/subscriptions_controller.rb +++ b/app/controllers/alaveteli_pro/subscriptions_controller.rb @@ -55,8 +55,6 @@ def create rescue Stripe::CardError => e flash[:error] = e.message - redirect_to plan_path(non_namespaced_plan_id) - return rescue Stripe::RateLimitError, Stripe::InvalidRequestError, @@ -78,7 +76,9 @@ def create _('There was a problem submitting your payment. You ' \ 'have not been charged. Please try again later.') end + end + if flash[:error] redirect_to plan_path(non_namespaced_plan_id) return end From cf8caf18f86ad916c79676834ec916abba2218b5 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 6 Sep 2019 17:02:38 +0100 Subject: [PATCH 030/124] Remove old_card_id params This is only used in our specs which can be written instead. --- .../payment_methods_controller.rb | 1 - .../subscriptions/_card.html.erb | 2 - .../payment_methods_controller_spec.rb | 66 ++++--------------- 3 files changed, 13 insertions(+), 56 deletions(-) diff --git a/app/controllers/alaveteli_pro/payment_methods_controller.rb b/app/controllers/alaveteli_pro/payment_methods_controller.rb index 0c3d925812..d55859cc2a 100644 --- a/app/controllers/alaveteli_pro/payment_methods_controller.rb +++ b/app/controllers/alaveteli_pro/payment_methods_controller.rb @@ -5,7 +5,6 @@ class AlaveteliPro::PaymentMethodsController < AlaveteliPro::BaseController def update begin @token = Stripe::Token.retrieve(params[:stripeToken]) - @old_card_id = params[:old_card_id] @pro_account = current_user.pro_account ||= current_user.build_pro_account @pro_account.source = @token.id diff --git a/app/views/alaveteli_pro/subscriptions/_card.html.erb b/app/views/alaveteli_pro/subscriptions/_card.html.erb index 02742c0f65..47368b07b6 100644 --- a/app/views/alaveteli_pro/subscriptions/_card.html.erb +++ b/app/views/alaveteli_pro/subscriptions/_card.html.erb @@ -15,8 +15,6 @@
<%= form_tag(payment_method_path, method: :put) do %> - <%= hidden_field_tag 'old_card_id', card.id %> - <%= stripe_button( label: _('Update Card Details'), panel_label: _('Update Card Details') diff --git a/spec/controllers/alaveteli_pro/payment_methods_controller_spec.rb b/spec/controllers/alaveteli_pro/payment_methods_controller_spec.rb index 9936610c50..f82e3c3846 100644 --- a/spec/controllers/alaveteli_pro/payment_methods_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/payment_methods_controller_spec.rb @@ -41,52 +41,32 @@ customer end - let(:old_card_id) { customer.sources.first.id } + let!(:card_ids) { customer.sources.data.map(&:id) } before do session[:user_id] = user.id end it 'finds the card token' do - post :update, params: { - 'stripeToken' => new_token, - 'old_card_id' => old_card_id - } + post :update, params: { 'stripeToken' => new_token } expect(assigns(:token).id).to eq(new_token) end - it 'finds the id of the card being updated' do - post :update, params: { - 'stripeToken' => new_token, - 'old_card_id' => old_card_id - } - expect(assigns(:old_card_id)).to eq(old_card_id) - end - it 'retrieves the correct pro account' do - post :update, params: { - 'stripeToken' => new_token, - 'old_card_id' => old_card_id - } + post :update, params: { 'stripeToken' => new_token } expect(assigns(:pro_account).stripe_customer_id). to eq(user.pro_account.stripe_customer_id) end it 'redirects to the account page' do - post :update, params: { - 'stripeToken' => new_token, - 'old_card_id' => old_card_id - } + post :update, params: { 'stripeToken' => new_token } expect(response).to redirect_to(subscriptions_path) end context 'with a successful transaction' do before do - post :update, params: { - 'stripeToken' => new_token, - 'old_card_id' => old_card_id - } + post :update, params: { 'stripeToken' => new_token } end it 'adds the new card to the Stripe customer' do @@ -105,8 +85,7 @@ it 'removes the old card from the Stripe customer' do reloaded = Stripe::Customer. retrieve(user.pro_account.stripe_customer_id) - expect(reloaded.sources.data.map(&:id)). - to_not include(old_card_id) + expect(reloaded.sources.data.map(&:id)).to_not match_array(card_ids) end it 'shows a message to confirm the update' do @@ -120,10 +99,7 @@ before do StripeMock.prepare_card_error(:card_declined, :update_customer) - post :update, params: { - 'stripeToken' => new_token, - 'old_card_id' => old_card_id - } + post :update, params: { 'stripeToken' => new_token } end it 'renders the card error message' do @@ -133,8 +109,7 @@ it 'does not update the stored payment methods' do reloaded = Stripe::Customer. retrieve(user.pro_account.stripe_customer_id) - expect(reloaded.sources.data.map(&:id)). - to include(old_card_id) + expect(reloaded.sources.data.map(&:id)).to match_array(card_ids) end end @@ -144,10 +119,7 @@ before do error = Stripe::RateLimitError.new StripeMock.prepare_error(error, :update_customer) - post :update, params: { - 'stripeToken' => new_token, - 'old_card_id' => old_card_id - } + post :update, params: { 'stripeToken' => new_token } end it 'sends an exception email' do @@ -166,10 +138,7 @@ before do error = Stripe::InvalidRequestError.new('message', 'param') StripeMock.prepare_error(error, :update_customer) - post :update, params: { - 'stripeToken' => new_token, - 'old_card_id' => old_card_id - } + post :update, params: { 'stripeToken' => new_token } end it 'sends an exception email' do @@ -188,10 +157,7 @@ before do error = Stripe::AuthenticationError.new StripeMock.prepare_error(error, :update_customer) - post :update, params: { - 'stripeToken' => new_token, - 'old_card_id' => old_card_id - } + post :update, params: { 'stripeToken' => new_token } end it 'sends an exception email' do @@ -210,10 +176,7 @@ before do error = Stripe::APIConnectionError.new StripeMock.prepare_error(error, :update_customer) - post :update, params: { - 'stripeToken' => new_token, - 'old_card_id' => old_card_id - } + post :update, params: { 'stripeToken' => new_token } end it 'sends an exception email' do @@ -232,10 +195,7 @@ before do error = Stripe::StripeError.new StripeMock.prepare_error(error, :update_customer) - post :update, params: { - 'stripeToken' => new_token, - 'old_card_id' => old_card_id - } + post :update, params: { 'stripeToken' => new_token } end it 'sends an exception email' do From 8098fe09a59a24c267da783d53542940d841617b Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 16 Aug 2019 20:58:00 +0100 Subject: [PATCH 031/124] Migrate to Stripe elements subscription form --- app/assets/config/manifest.js | 2 + .../javascripts/alaveteli_pro/stripe.js | 110 ++++++++++++++ .../responsive/_settings_style.scss | 1 + .../responsive/alaveteli_pro/_stripe.scss | 19 +++ app/assets/stylesheets/responsive/all.scss | 2 + .../alaveteli_pro/subscriptions_controller.rb | 6 +- app/helpers/stripe_helper.rb | 12 ++ app/views/alaveteli_pro/plans/show.html.erb | 58 +++----- app/views/layouts/default.html.erb | 1 + .../subscriptions_controller_spec.rb | 140 +++++++----------- spec/helpers/stripe_helper_spec.rb | 28 ++++ 11 files changed, 257 insertions(+), 122 deletions(-) create mode 100644 app/assets/javascripts/alaveteli_pro/stripe.js create mode 100644 app/assets/stylesheets/responsive/alaveteli_pro/_stripe.scss diff --git a/app/assets/config/manifest.js b/app/assets/config/manifest.js index bc43cf9565..cb13379c27 100644 --- a/app/assets/config/manifest.js +++ b/app/assets/config/manifest.js @@ -26,3 +26,5 @@ //= link responsive/print.css //= link responsive/application-lte-ie7.css //= link responsive/application-ie8.css +// +//= link alaveteli_pro/stripe.js diff --git a/app/assets/javascripts/alaveteli_pro/stripe.js b/app/assets/javascripts/alaveteli_pro/stripe.js new file mode 100644 index 0000000000..cc3e3b1b15 --- /dev/null +++ b/app/assets/javascripts/alaveteli_pro/stripe.js @@ -0,0 +1,110 @@ +(function() { + 'use strict'; + + var subscriptionForm = document.getElementById('js-stripe-subscription-form'); + if (subscriptionForm) { stripeForm(form, {}) } +})(); + +function stripeForm(form, options) { + var that = Object.assign({ + stripe: Stripe(AlaveteliPro.stripe_publishable_key), + form: form, + submit: document.getElementById('js-stripe-submit'), + }, options); + + that.load = function() { + var cardError = document.getElementById('card-errors'); + var terms = document.getElementById('js-pro-signup-terms'); + + // Sync initial state for terms checkbox and submit button + that.submit.setAttribute('disabled', 'true'); + terms.checked = false; + + // Initialise Stripe Elements + var elements = that.stripe.elements({ locale: AlaveteliPro.stripe_locale }); + + // Create an instance of the card Element. + var style = { base: { fontSize: '16px' } }; + var card = elements.create('card', { style: style }); + + // Add an instance of the card Element into the `card-element`
. + card.mount('#card-element'); + + // Listen to change events on the card Element and display any errors + card.addEventListener('change', function(event) { + if (event.error) { + cardError.textContent = event.error.message; + } else { + cardError.textContent = ''; + } + + that.submitConditions.cardValid = !(event.error); + that.updateSubmit(); + }); + + terms.addEventListener('click', function(event) { + if (this.checked) { + that.submitConditions.termAccepted = true; + } else { + that.submitConditions.termAccepted = false; + } + that.updateSubmit(); + }); + + // Create a token or display an error when the form is submitted + that.form.addEventListener('submit', function(event) { + event.preventDefault(); + + // Ensure submit conditions are met + if (!that.canSubmit()) { return false; } + + that.stripe.createToken(card).then(function(result) { + if (result.error) { + // Inform the customer that there was an error + cardError.textContent = result.error.message; + + // Prevent re-submitting after error + that.submitConditions.cardValid = false; + that.updateSubmit(); + + // Reset submit button value which was changed by Rails' UJS + // disable-with option + var text = $(that.submit).data('ujs:enable-with'); + if (text) { $(that.submit)['val'](text); } + } else { + // Send the token to your server. + that.stripeTokenHandler(result.token); + } + }); + }); + }; + + that.canSubmit = function() { + for (var condition in that.submitConditions) { + if(!that.submitConditions[condition]) { return false; } + } + return true; + }; + + that.updateSubmit = function() { + if (that.canSubmit()) { + that.submit.removeAttribute('disabled'); + } else { + that.submit.setAttribute('disabled', 'true'); + } + }; + + that.stripeTokenHandler = function(token) { + // Insert the token ID into the form so it gets submitted to the server + var hiddenInput = document.createElement('input'); + hiddenInput.setAttribute('type', 'hidden'); + hiddenInput.setAttribute('name', 'stripe_token'); + hiddenInput.setAttribute('value', token.id); + that.form.appendChild(hiddenInput); + + // Submit the form + that.form.submit(); + }; + + that.load(); +} diff --git a/app/assets/stylesheets/responsive/_settings_style.scss b/app/assets/stylesheets/responsive/_settings_style.scss index 34a2589337..7f8c13daea 100644 --- a/app/assets/stylesheets/responsive/_settings_style.scss +++ b/app/assets/stylesheets/responsive/_settings_style.scss @@ -57,6 +57,7 @@ .settings__cancel-button { color: #333; font-size: 0.875em; + margin-left: 1em; } .pricing__faqs { diff --git a/app/assets/stylesheets/responsive/alaveteli_pro/_stripe.scss b/app/assets/stylesheets/responsive/alaveteli_pro/_stripe.scss new file mode 100644 index 0000000000..bd2612ea30 --- /dev/null +++ b/app/assets/stylesheets/responsive/alaveteli_pro/_stripe.scss @@ -0,0 +1,19 @@ +form.stripe-form { + #card-element { + border-color: #BBB; + border-style: solid; + border-width: 1px; + box-shadow: inset 0 1px 2px rgba(0, 0, 0, 0.1); + color: rgba(0, 0, 0, 0.75); + border-radius: 0; + background-color: #ffffff; + padding: 5px; + margin-bottom: 1.5em; + } + + #card-errors { + padding-top: 0.5em; + height: 1.5em; + color: red; + } +} diff --git a/app/assets/stylesheets/responsive/all.scss b/app/assets/stylesheets/responsive/all.scss index 8de895440a..7794d3aac2 100644 --- a/app/assets/stylesheets/responsive/all.scss +++ b/app/assets/stylesheets/responsive/all.scss @@ -102,6 +102,8 @@ @import "responsive/alaveteli_pro/_pro_marketing"; +@import "responsive/alaveteli_pro/_stripe"; + @import "responsive/_ms_edge"; @import "responsive/custom"; diff --git a/app/controllers/alaveteli_pro/subscriptions_controller.rb b/app/controllers/alaveteli_pro/subscriptions_controller.rb index 49f093a682..b331b6abf3 100644 --- a/app/controllers/alaveteli_pro/subscriptions_controller.rb +++ b/app/controllers/alaveteli_pro/subscriptions_controller.rb @@ -29,15 +29,13 @@ def index # params => # {"utf8"=>"✓", # "authenticity_token"=>"Ono2YgLcl1eC1gGzyd7Vf5HJJhOek31yFpT+8z+tKoo=", - # "stripeToken"=>"tok_s3kr3t…", - # "stripeTokenType"=>"card", - # "stripeEmail"=>"bob@example.com", + # "stripe_token"=>"tok_s3kr3t…", # "controller"=>"alaveteli_pro/subscriptions", # "action"=>"create", # "plan_id"=>"WDTK-pro"} def create begin - @token = Stripe::Token.retrieve(params[:stripeToken]) + @token = Stripe::Token.retrieve(params[:stripe_token]) @pro_account = current_user.pro_account ||= current_user.build_pro_account @pro_account.source = @token.id diff --git a/app/helpers/stripe_helper.rb b/app/helpers/stripe_helper.rb index 55727f806c..33fd7a2fa7 100644 --- a/app/helpers/stripe_helper.rb +++ b/app/helpers/stripe_helper.rb @@ -1,10 +1,22 @@ module StripeHelper + STRIPE_SUPPORTED_LOCALES = %w[ + da de en es fi fr it ja nb nl pl pt sv zh + ].freeze + def stripe_button(options = {}) content_tag :script, '', stripe_button_default_options.deep_merge( data: options ) end + def stripe_locale + if STRIPE_SUPPORTED_LOCALES.include?(@locales[:current]) + @locales[:current] + else + 'auto' + end + end + private def stripe_button_default_options diff --git a/app/views/alaveteli_pro/plans/show.html.erb b/app/views/alaveteli_pro/plans/show.html.erb index 264bb17e6f..b450621dca 100644 --- a/app/views/alaveteli_pro/plans/show.html.erb +++ b/app/views/alaveteli_pro/plans/show.html.erb @@ -1,27 +1,14 @@ -<% content_for :javascript do %> - +<% end %> - $(function() { - $('#accept-terms').click(function() { - if ($(this).is(':checked')) { - $('#pro-signup button').removeAttr('disabled'); - } else { - $('#pro-signup button').attr('disabled', 'disabled'); - } - }); - }); +<% content_for :javascript do %> + <%= javascript_tag do %> + AlaveteliPro.stripe_publishable_key = '<%= AlaveteliConfiguration.stripe_publishable_key %>'; + AlaveteliPro.stripe_local = '<%= stripe_locale %>'; + <% end %> - $(function() { - $('#pro-signup button span').click(function() { - if ($('#pro-signup button').is(':disabled')) { - alert('<%=_('You must accept the terms and conditions') %>'); - } - }); - }); - + <%= javascript_include_tag 'alaveteli_pro/stripe' %> <% end %>
@@ -37,7 +24,7 @@
- <%= form_tag(subscriptions_path, id: 'pro-signup') do %> + <%= form_tag(subscriptions_path, id: 'js-stripe-subscription-form', class: 'stripe-form') do %>

<%= _('Selected plan') %>

@@ -53,10 +40,17 @@
+
+ +
<%= text_field_tag 'coupon_code', params[:coupon_code] %>
+
+ +
+
@@ -67,23 +61,18 @@
-
- <%= hidden_field_tag 'plan_id', @plan.id %> - - <%= stripe_button( - label: _('Pay with card'), - panel_label: _('Pay'), - description: @stripe_button_description, - amount: @plan.amount_with_tax, - currency: AlaveteliConfiguration.iso_currency_code - ) %> +
+ <%= hidden_field_tag 'plan_id', @plan.id %> + <%= submit_tag _('Subscribe'), id: 'js-stripe-submit', disabled: true, data: { disable_with: 'Processing...' } %> <%= link_to _('Cancel'), pro_plans_path, class: 'settings__cancel-button' %> +

+
<% end %> <% if feature_enabled?(:pro_pricing_faqs) %> diff --git a/app/views/layouts/default.html.erb b/app/views/layouts/default.html.erb index 55b4ec343f..ba0830229c 100644 --- a/app/views/layouts/default.html.erb +++ b/app/views/layouts/default.html.erb @@ -43,6 +43,7 @@ H.className = H.className.replace(/\bno-js\b/,'js-loaded') })(document.documentElement); + <%= content_for :javascript_head %> <% if is_admin? %> diff --git a/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb b/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb index 33372aef5d..655a484912 100644 --- a/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb +++ b/spec/controllers/alaveteli_pro/subscriptions_controller_spec.rb @@ -101,19 +101,15 @@ before do session[:user_id] = user.id post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => '' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => '' + } post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => '' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => '' + } end it 'does not create a duplicate subscription' do @@ -131,12 +127,10 @@ context 'with a successful transaction' do before do post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => '' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => '' + } end include_examples 'successful example' @@ -145,12 +139,10 @@ context 'with coupon code' do before do post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => 'coupon_code' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => 'coupon_code' + } end include_examples 'successful example' @@ -166,12 +158,10 @@ and_return('alaveteli') post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => 'coupon_code' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => 'coupon_code' + } end include_examples 'successful example' @@ -191,12 +181,10 @@ user.create_pro_account(:stripe_customer_id => customer.id) post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => '' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => '' + } end include_examples 'successful example' @@ -217,12 +205,10 @@ before do StripeMock.prepare_card_error(:card_declined, :create_subscription) post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => '' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => '' + } end it 'renders the card error message' do @@ -245,12 +231,10 @@ error = Stripe::RateLimitError.new StripeMock.prepare_error(error, :create_subscription) post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => '' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => '' + } end it 'sends an exception email' do @@ -274,12 +258,10 @@ error = Stripe::InvalidRequestError.new('message', 'param') StripeMock.prepare_error(error, :create_subscription) post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => '' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => '' + } end it 'sends an exception email' do @@ -303,12 +285,10 @@ error = Stripe::AuthenticationError.new StripeMock.prepare_error(error, :create_subscription) post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => '' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => '' + } end it 'sends an exception email' do @@ -332,12 +312,10 @@ error = Stripe::APIConnectionError.new StripeMock.prepare_error(error, :create_subscription) post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => '' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => '' + } end it 'sends an exception email' do @@ -361,12 +339,10 @@ error = Stripe::StripeError.new StripeMock.prepare_error(error, :create_subscription) post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => '' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => '' + } end it 'sends an exception email' do @@ -390,12 +366,10 @@ error = Stripe::InvalidRequestError.new('No such coupon', 'param') StripeMock.prepare_error(error, :create_subscription) post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => 'INVALID' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => 'INVALID' + } end it 'does not sends an exception email' do @@ -419,12 +393,10 @@ error = Stripe::InvalidRequestError.new('Coupon expired', 'param') StripeMock.prepare_error(error, :create_subscription) post :create, params: { - 'stripeToken' => token, - 'stripeTokenType' => 'card', - 'stripeEmail' => user.email, - 'plan_id' => 'pro', - 'coupon_code' => 'EXPIRED' - } + 'stripe_token' => token, + 'plan_id' => 'pro', + 'coupon_code' => 'EXPIRED' + } end it 'does not sends an exception email' do diff --git a/spec/helpers/stripe_helper_spec.rb b/spec/helpers/stripe_helper_spec.rb index 9f35dc2fa7..36b89b339a 100644 --- a/spec/helpers/stripe_helper_spec.rb +++ b/spec/helpers/stripe_helper_spec.rb @@ -48,4 +48,32 @@ end + describe '#stripe_locale' do + + class MockHelper + include StripeHelper + + def initialize(locale) + @locales = { current: locale } + end + end + + subject { MockHelper.new(locale).stripe_locale } + + context 'current local supported by Stripe' do + + let(:locale) { 'en' } + it { is_expected.to eq 'en' } + + end + + context 'current local not supported by Stripe' do + + let(:locale) { 'cy' } + it { is_expected.to eq 'auto' } + + end + + end + end From 4ddce2b61f136d2bf087f8b8f5809cad8ea2c258 Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Fri, 6 Sep 2019 16:30:24 +0100 Subject: [PATCH 032/124] Migrate to Stripe elements add/update card form --- .../javascripts/alaveteli_pro/stripe.js | 47 ++++++++++++++----- .../payment_methods_controller.rb | 2 +- .../subscriptions/_add_card.html.erb | 11 ++--- .../subscriptions/_card.html.erb | 16 +++---- .../subscriptions/index.html.erb | 13 +++++ .../payment_methods_controller_spec.rb | 18 +++---- 6 files changed, 71 insertions(+), 36 deletions(-) diff --git a/app/assets/javascripts/alaveteli_pro/stripe.js b/app/assets/javascripts/alaveteli_pro/stripe.js index cc3e3b1b15..3f5b0bec70 100644 --- a/app/assets/javascripts/alaveteli_pro/stripe.js +++ b/app/assets/javascripts/alaveteli_pro/stripe.js @@ -2,23 +2,52 @@ 'use strict'; var subscriptionForm = document.getElementById('js-stripe-subscription-form'); - if (subscriptionForm) { stripeForm(form, {}) } + if (subscriptionForm) { stripeSubscriptionForm(subscriptionForm); } + + var updateForm = document.getElementById('js-stripe-update-form'); + if (updateForm) { stripeForm(form, {}) } })(); +function stripeSubscriptionForm(form) { + stripeForm(form, { + submitConditions: { cardValid: false, termAccepted: false }, + setupCallback: function (that) { + var terms = document.getElementById('js-pro-signup-terms'); + + // Sync initial state for terms checkbox and submit button + terms.checked = false; + + // Enable submit button if terms are accepted + terms.addEventListener('click', function(event) { + if (this.checked) { + that.submitConditions.termAccepted = true; + } else { + that.submitConditions.termAccepted = false; + } + that.updateSubmit(); + }); + } + }) +} + function stripeForm(form, options) { var that = Object.assign({ stripe: Stripe(AlaveteliPro.stripe_publishable_key), form: form, submit: document.getElementById('js-stripe-submit'), + + // Conditions which must be met before we submit the form + submitConditions: { cardValid: false }, + + // Callback for customisation + setupCallback: function() {}, }, options); that.load = function() { var cardError = document.getElementById('card-errors'); - var terms = document.getElementById('js-pro-signup-terms'); // Sync initial state for terms checkbox and submit button that.submit.setAttribute('disabled', 'true'); - terms.checked = false; // Initialise Stripe Elements var elements = that.stripe.elements({ locale: AlaveteliPro.stripe_locale }); @@ -30,6 +59,9 @@ function stripeForm(form, options) { // Add an instance of the card Element into the `card-element`
. card.mount('#card-element'); + // Call callback to allow addition setup + that.setupCallback(that); + // Listen to change events on the card Element and display any errors card.addEventListener('change', function(event) { if (event.error) { @@ -42,15 +74,6 @@ function stripeForm(form, options) { that.updateSubmit(); }); - terms.addEventListener('click', function(event) { - if (this.checked) { - that.submitConditions.termAccepted = true; - } else { - that.submitConditions.termAccepted = false; - } - that.updateSubmit(); - }); - // Create a token or display an error when the form is submitted that.form.addEventListener('submit', function(event) { event.preventDefault(); diff --git a/app/controllers/alaveteli_pro/payment_methods_controller.rb b/app/controllers/alaveteli_pro/payment_methods_controller.rb index d55859cc2a..d9048ff1ac 100644 --- a/app/controllers/alaveteli_pro/payment_methods_controller.rb +++ b/app/controllers/alaveteli_pro/payment_methods_controller.rb @@ -4,7 +4,7 @@ class AlaveteliPro::PaymentMethodsController < AlaveteliPro::BaseController def update begin - @token = Stripe::Token.retrieve(params[:stripeToken]) + @token = Stripe::Token.retrieve(params[:stripe_token]) @pro_account = current_user.pro_account ||= current_user.build_pro_account @pro_account.source = @token.id diff --git a/app/views/alaveteli_pro/subscriptions/_add_card.html.erb b/app/views/alaveteli_pro/subscriptions/_add_card.html.erb index 97acd74404..0bb8427da5 100644 --- a/app/views/alaveteli_pro/subscriptions/_add_card.html.erb +++ b/app/views/alaveteli_pro/subscriptions/_add_card.html.erb @@ -1,16 +1,15 @@
- <%= form_tag(payment_method_path, method: :put) do %> + <%= form_tag(payment_method_path, id: 'js-stripe-update-form', class: 'stripe-form', method: :put) do %> +
- <%= stripe_button( - label: _('Add Payment Card'), - panel_label: _('Add Payment Card') - ) %> + <%= submit_tag _('Add Card Details'), id: 'js-stripe-submit', disabled: true, data: { disable_with: 'Processing...' } %> +