Async csv import (#28)

* Extract RevokeUser command

And I added more tests covering missing cases while at it.

* Refactor command

To simplify and increase readability

* Move tests to unit-level and cover more cases

* Check if object exists instead of instantiating

It's not worth creating an unpersisted authorization just to avoid
checking if we there was one already. It was a nice trick but it's not
worth the cost of instantiating a model IMO.

* Do not swallow exceptions revoking authorization

First, given `RevokeUser` was already checking for the existence of
authorization, the logic on `DestroyUserAuthorization` that did the same
was pointless. At the point we hit it, there's always an authorization.
As a result, and because there was no other `:invalid` scenario the
`:invalid` callback was never called. Precisely, it was broken and no
test failed.

All the other possible failure scenarios will be caused by
`authorization.destroy!` but rescue to track the error
`instrumenter.add_error` would hide any exceptional behaviour we should
be aware of and see in the app's error tracker. If a DB record can't be
deleted it's likely that something is really wrong.

* Extract Instrumenter class out of UserProcessor

This a separate responsibility that deserves its own class and which
hampers UserProcessor changeability.

* Memomize user

* Abstract instrumenter to not leak data structures

This more abstract public API will enable us to provide different
implementations of instrumenter without having to touch its consumers.

* Replace uniqueness logic with Ruby Set data type

Ruby keeps uniqueness for us 😎

* Move methods within their class

* Create job to register a list of users

Note we make use of ActiveJob's GlobalIDs instead of *_id arguments as
we're used to.

* Enable importing a CSV from a new page

* Send email notification when users registered ok

* Enforce permissions to import

* Add error handling through form

* Replace test CSV-building logic with file fixture

* Map user input actions and test form

* Improve registers users specs

* Set up mail previews

* Use Decidim's layout which includes org's info

* Show user registration stats in email

As we show the count of success and failures there's no need to a have
a separate email notification to send on error.

* Revoke users async

* Extract base job class and use template methods

This makes the algorithm more obvious and job classes less boilerplate-ish.

* Use Decidim's client-side validation in form

* Validate presence of :authorize as string

* Remove unnecessary form label

* Display CSV format template to improve UX

A better solution is to give users a CSV file template they can download
and modify.

* Fix leading whitespace by removing I18n-ability

I changed my mind. It's not worth making this translatable at this early
stage and I find this leading whitespace worse than having only an
English version of the template.

* Move email delivery responsibility to base job

This makes concrete user processing job classes even simpler.

* Enable registering and authorizing users async

This fixes the confusion we had with how the registering and authorizing
actions are sent from the form. The former is optional but authorizing,
revoking and checking come from radio buttons so there'll always come
one in the request. Because the radio button enabled by default is
"check", if only registering is select we'll only register users. See
`create_import_form_spec.rb` for all the permutations.

Note we don't do any processing when "check" is provided yet.

* Decouple callers from the name fetching details

* Use users sidebar in imports/new page

* Allow trailing and leading whitespaces in CSVs

The extra whitespace caused `RegisterUser`'s form to return a nil
`#name` and thus, the fallback name was used although the CSV contained
a name column.

While at it, I also tried to make the system tests more closely mimic
the manual acceptance test I'm doing when checking the email in my
inbox. That's how I noticed the `Hola saulopefa+ava_test,` wasn't what
I expected.

* Replace DB checks with UI checks in system specs

This makes these specs and thus the app more resilient. In the end, in
a manual acceptance test we would have no other option than navigating
to the authorizations page to see whether or not the authorization is
granted.

* Improve file import mail subject

* Fix parsing empty columns

Closes CoopCat-Confederacio-de-Cooperatives/decidim-coopcat#132.

* Set hash value nil when no column is provided

Better not to swallow it, so we can still trace the error if the user
complains. We'll be able to see the authorization DB record and see the
input was broken.

* DRY and make spec contexts more evident

* Navigate to CSV import from /direct_verifications

Closes CoopCat-Confederacio-de-Cooperatives/decidim-coopcat#134.

This simply makes it possible to reach the
/direct_verifications/imports/new page that was previously introduced
and demoed to ICA; the customer requesting this feature.

* Serialize file path instead of its contents

There's no way passing the file contents as a serialized argument could
end well.

* Revert "Merge pull request #23 from coopdevs/dont-serialize-file-contents"

This reverts commit 9e30bad, reversing
changes made to a6f4dcb.

* Show error when no CSV header is provided

This fixes
https://sentry.io/organizations/cercles-coop/issues/2479093815.

We infer the user didn't provide a header row (which happens very often)
when any of the first row columns are empty. This is a much better UX
than silent 500 error. The user will email us at best or simply abandon
the job.

* Provide CA and ES translations of #25

* Add authorization_handler field in CSV import page

* Make RevokeUser work with authorization handlers

This fixes the regression introduced in 5cc20be.

* Make AuthorizeUser work w/ authorization handlers

This fixes the regression introduced in 5cc20be.

* Improve tests of registration use case

Making the authorization_handler argument mandatory also makes things
a bit more clear, because there's no conditional.

* Make handler argument mandatory in RevokeUser

Removing that conditional bit makes things a bit more clear and robust.

app/commands/decidim/direct_verifications/verification/create_import.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+module Decidim
+  module DirectVerifications
+    module Verification
+      class CreateImport < Rectify::Command
+        def initialize(form)
+          @form = form
+          @file = form.file
+          @organization = form.organization
+          @user = form.user
+          @action = form.action
+        end
+
+        def call
+          return broadcast(:invalid) unless form.valid?
+
+          case action
+          when :register
+            register_users_async
+          when :register_and_authorize
+            register_users_async
+            file.rewind
+            authorize_users_async
+          when :revoke
+            revoke_users_async
+          end
+
+          broadcast(:ok)
+        end
+
+        private
+
+        attr_reader :form, :file, :organization, :user, :action
+
+        def register_users_async
+          RegisterUsersJob.perform_later(file.read, organization, user, form.authorization_handler)
+        end
+
+        def revoke_users_async
+          RevokeUsersJob.perform_later(file.read, organization, user, form.authorization_handler)
+        end
+
+        def authorize_users_async
+          AuthorizeUsersJob.perform_later(file.read, organization, user, form.authorization_handler)
+        end
+      end
+    end
+  end
+end

app/commands/decidim/direct_verifications/verification/metadata_parser.rb

@@ -9,8 +9,8 @@ class MetadataParser < BaseParser
         def header
           @header ||= begin
                         header_row = lines[0].chomp
-                        column_names = tokenize(header_row)
-                        column_names.map(&:to_sym).map(&:downcase)
+                        header_row = tokenize(header_row)
+                        normalize_header(header_row)
                       end
         end
 
@@ -23,8 +23,8 @@ def parse_data(email, line, header)
 
           hash = {}
           header.each_with_index do |column, index|
-            value = tokens[index].strip
-            next if value.include?(email)
+            value = tokens[index]
+            next if value&.include?(email)
 
             hash[column] = value
           end
@@ -34,7 +34,17 @@ def parse_data(email, line, header)
         private
 
         def tokenize(line)
-          CSV.parse(line)[0]
+          CSV.parse_line(line).map do |token|
+            token&.strip
+          end
+        end
+
+        def normalize_header(line)
+          line.map do |field|
+            raise MissingHeaderError if field.nil?
+
+            field.to_sym.downcase
+          end
         end
       end
     end

app/commands/decidim/direct_verifications/verification/missing_header_error.rb

@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+module Decidim
+  module DirectVerifications
+    module Verification
+      class MissingHeaderError < StandardError
+      end
+    end
+  end
+end

app/controllers/decidim/direct_verifications/verification/admin/direct_verifications_controller.rb

@@ -10,6 +10,8 @@ class DirectVerificationsController < Decidim::Admin::ApplicationController
 
           layout "decidim/admin/users"
 
+          I18N_SCOPE = "decidim.direct_verifications.verification.admin.direct_verifications"
+
           def index
             enforce_permission_to :index, :authorization
           end
@@ -18,29 +20,39 @@ def create
             enforce_permission_to :create, :authorization
 
             @userslist = params[:userslist]
-            @processor = UserProcessor.new(current_organization, current_user, session)
+
+            @processor = UserProcessor.new(current_organization, current_user, session, instrumenter)
             @processor.emails = parser_class.new(@userslist).to_h
             @processor.authorization_handler = current_authorization_handler
+
             @stats = UserStats.new(current_organization)
             @stats.authorization_handler = @processor.authorization_handler
+
             register_users
             authorize_users
             revoke_users
 
             render(action: :index) && return if show_users_info
 
+            redirect_to direct_verifications_path
+          rescue MissingHeaderError => _e
+            flash[:error] = I18n.t("#{I18N_SCOPE}.create.missing_header")
             redirect_to direct_verifications_path
           end
 
           private
 
+          def instrumenter
+            @instrumenter ||= Instrumenter.new(current_user)
+          end
+
           def register_users
             return unless params[:register]
 
             @processor.register_users
             flash[:warning] = t(".registered", count: @processor.emails.count,
-                                               registered: @processor.processed[:registered].count,
-                                               errors: @processor.errors[:registered].count)
+                                               registered: instrumenter.processed_count(:registered),
+                                               errors: instrumenter.errors_count(:registered))
           end
 
           def authorize_users
@@ -49,8 +61,8 @@ def authorize_users
             @processor.authorize_users
             flash[:notice] = t(".authorized", handler: t("#{@processor.authorization_handler}.name", scope: "decidim.authorization_handlers"),
                                               count: @processor.emails.count,
-                                              authorized: @processor.processed[:authorized].count,
-                                              errors: @processor.errors[:authorized].count)
+                                              authorized: instrumenter.processed_count(:authorized),
+                                              errors: instrumenter.errors_count(:authorized))
           end
 
           def revoke_users
@@ -59,8 +71,8 @@ def revoke_users
             @processor.revoke_users
             flash[:notice] = t(".revoked", handler: t("#{@processor.authorization_handler}.name", scope: "decidim.authorization_handlers"),
                                            count: @processor.emails.count,
-                                           revoked: @processor.processed[:revoked].count,
-                                           errors: @processor.errors[:revoked].count)
+                                           revoked: instrumenter.processed_count(:revoked),
+                                           errors: instrumenter.errors_count(:revoked))
           end
 
           def show_users_info

app/controllers/decidim/direct_verifications/verification/admin/imports_controller.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+module Decidim
+  module DirectVerifications
+    module Verification
+      module Admin
+        class ImportsController < Decidim::Admin::ApplicationController
+          layout "decidim/admin/users"
+          helper_method :workflows, :current_authorization_handler
+
+          def new
+            enforce_permission_to :create, :authorization
+            @form = form(CreateImportForm).instance
+          end
+
+          def create
+            enforce_permission_to :create, :authorization
+
+            defaults = { organization: current_organization, user: current_user }
+            form = form(CreateImportForm).from_params(params.merge(defaults))
+
+            CreateImport.call(form) do
+              on(:ok) do
+                flash[:notice] = t(".success")
+              end
+
+              on(:invalid) do
+                flash[:alert] = t(".error")
+              end
+            end
+
+            redirect_to new_import_path
+          end
+
+          def workflows
+            workflows = configured_workflows & current_organization.available_authorizations.map.to_a
+            workflows.map do |workflow|
+              [t("#{workflow}.name", scope: "decidim.authorization_handlers"), workflow]
+            end
+          end
+
+          def configured_workflows
+            return Decidim::DirectVerifications.config.manage_workflows if Decidim::DirectVerifications.config
+
+            ["direct_verifications"]
+          end
+
+          def current_authorization_handler
+            params[:authorization_handler]
+          end
+        end
+      end
+    end
+  end
+end

app/forms/decidim/direct_verifications/verification/create_import_form.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+module Decidim
+  module DirectVerifications
+    module Verification
+      class CreateImportForm < Form
+        ACTIONS = {
+          "in" => :authorize,
+          "out" => :revoke,
+          "check" => :check
+        }.freeze
+
+        attribute :file
+        attribute :organization, Decidim::Organization
+        attribute :user, Decidim::User
+        attribute :authorize, String
+        attribute :register, Boolean
+        attribute :authorization_handler, String
+
+        validates :file, :organization, :user, :authorize, :authorization_handler, presence: true
+        validates :authorize, inclusion: { in: ACTIONS.keys }
+
+        validate :available_authorization_handler
+
+        def available_authorization_handler
+          return if authorization_handler.in?(organization.available_authorizations)
+
+          errors.add(:authorization_handler, :inclusion)
+        end
+
+        def action
+          if register && authorize == "in"
+            :register_and_authorize
+          elsif register
+            :register
+          else
+            ACTIONS[authorize]
+          end
+        end
+      end
+    end
+  end
+end

app/jobs/decidim/direct_verifications/authorize_users_job.rb

@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require "decidim/direct_verifications/instrumenter"
+
+module Decidim
+  module DirectVerifications
+    class AuthorizeUsersJob < BaseImportJob
+      class NullSession; end
+
+      def process_users
+        emails.each do |email, data|
+          AuthorizeUser.new(
+            email,
+            data,
+            session,
+            organization,
+            instrumenter,
+            authorization_handler
+          ).call
+        end
+      end
+
+      def type
+        :authorized
+      end
+
+      private
+
+      def session
+        NullSession.new
+      end
+    end
+  end
+end

app/jobs/decidim/direct_verifications/base_import_job.rb

@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+require "decidim/direct_verifications/instrumenter"
+
+module Decidim
+  module DirectVerifications
+    # This class implements the logic to import the user entries and sending an email notification
+    # with the result. The specifics to process the entries are meant to be implemented by
+    # subclasses which must implement the `#process_users` and `#type` methods.
+    class BaseImportJob < ApplicationJob
+      queue_as :default
+
+      def perform(userslist, organization, current_user, authorization_handler)
+        @emails = Verification::MetadataParser.new(userslist).to_h
+        @organization = organization
+        @current_user = current_user
+        @instrumenter = Instrumenter.new(current_user)
+        @authorization_handler = authorization_handler
+
+        process_users
+        send_email_notification
+      end
+
+      private
+
+      attr_reader :emails, :organization, :current_user, :instrumenter, :authorization_handler
+
+      def send_email_notification
+        ImportMailer.finished_processing(
+          current_user,
+          instrumenter,
+          type,
+          authorization_handler
+        ).deliver_now
+      end
+    end
+  end
+end

app/jobs/decidim/direct_verifications/register_users_job.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+require "decidim/direct_verifications/instrumenter"
+
+module Decidim
+  module DirectVerifications
+    class RegisterUsersJob < BaseImportJob
+      def process_users
+        emails.each do |email, data|
+          RegisterUser.new(email, data, organization, current_user, instrumenter).call
+        end
+      end
+
+      def type
+        :registered
+      end
+    end
+  end
+end

app/jobs/decidim/direct_verifications/revoke_users_job.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module Decidim
+  module DirectVerifications
+    class RevokeUsersJob < BaseImportJob
+      def process_users
+        emails.each do |email, _name|
+          RevokeUser.new(email, organization, instrumenter, authorization_handler).call
+        end
+      end
+
+      def type
+        :revoked
+      end
+    end
+  end
+end