support for CSV and multiple columns (#2)

* Remove not found `url` gem

A `bundle install` complaints with:

```
Fetching gem metadata from https://rubygems.org/........
Fetching gem metadata from https://rubygems.org/.
Resolving dependencies....
Your bundle is locked to url (0.3.2), but that version could not be
found in any of the sources listed in your Gemfile. If you haven't
changed sources, that means the author of url (0.3.2) has removed it.
You'll need to update your bundle to a version
other than url (0.3.2) that hasn't been removed in order to install.
```

It looks like that the gem that depends on this one had the same issue
and drop it.

* Gitignore common files

* Extract email parsing into class

And move Parser's specs to a new class-specific file.

* Replace send calls to public method

No reason to use `#send` if the method is public.

* Improve a bit specs wording

* Accept email data as a hash and store as metadata

* Enable toggling the parser from config

This adds a new parser and while making it backward compatible. It'll
only be enabled if you configure with `processor = :metadata`.

It does so by extracting specific entry parsing into two classes. Now we
have them both encapsulated and we can extend the metadata one however
we want without affecting current behaviour.

* Read CSV format

Well, a bit rudimentary still, but also normalize a bit the entry
format. Less flexible => less complexity => less buggy.

* Accept CSV header and unlimited columns

This turns the entry parsers into full text parsers each with different
features, not only at the entry level. While we keep the simple parser
that deals only with names for backward compatibility, the new metadata
parser deals with CSV and unlimited columns, while being more strict in
its input format.

* Refactor parsers naming

So they better convey the use of the template method pattern and how
things are design around inheritance.

* Use stdlib's CSV interface

Ruby's CSV parser can't compare to our naïve `split(",")`.

* Increase #register_users test coverage

* Doc Metadata mode and better config key

Processor is already used by UserProcessor which is different from the
parsers, which may lead to confusion.

* Move email and name normalization to parser

This is where it belongs.

As a result, now the metadata parser doesn't force you to have the email
in the second column. It'll just keep it out of the metadata JSON,
that's all.

* Remove unnecessary chomp

The CSV lib already does so.

* Memoize header row parsing

We are hitting this computation for every single line from the CSV we
process.

* Fix flaky spec

Hopefully. It's now consistently passing but I don't know why. I also
took the chance to tidy up the shared examples hoping that'd make it
easier to find to root cause of the flakyness.

.gitignore

@@ -18,3 +18,7 @@ spec/decidim_dummy_app
 
 # default development application
 development_app
+
+.byebug_history
+.ruby-version
+tags

Gemfile.lock

@@ -109,10 +109,9 @@ GEM
       cells (>= 4.1.6, < 5.0.0)
     charlock_holmes (0.7.7)
     childprocess (3.0.0)
-    codecov (0.1.16)
+    codecov (0.2.11)
       json
       simplecov
-      url
     coercible (1.0.0)
       descendants_tracker (~> 0.0.1)
     coffee-rails (5.0.0)
@@ -428,7 +427,7 @@ GEM
       thor (>= 0.14, < 2.0)
     jquery-tmpl-rails (1.1.0)
       rails (>= 3.1.0)
-    json (2.3.0)
+    json (2.3.1)
     jwt (2.2.1)
     kaminari (1.2.0)
       activesupport (>= 4.1.0)
@@ -708,7 +707,6 @@ GEM
       unf_ext
     unf_ext (0.0.7.7)
     unicode-display_width (1.6.1)
-    url (0.3.2)
     valid_email2 (2.3.1)
       activemodel (>= 3.2)
       mail (~> 2.5)
@@ -757,7 +755,7 @@ DEPENDENCIES
   web-console (~> 3.5)
 
 RUBY VERSION
-   ruby 2.6.3p62
+   ruby 2.6.6p146
 
 BUNDLED WITH
    2.1.4

README.md

@@ -37,6 +37,21 @@ With the detected list of emails admin have different options available:
 3. Revoke the authorization for the list of users using any verification method available.
 4. Check the status of the users in order to know if they are verified or registered.
 
+### Metadata mode
+
+This mode provides extra capabilities over the default processing:
+
+* Reads CSV format with header (copy and paste it from your spreadsheet)
+* Stores all columns except the email as authorization metadata
+
+This enables querying the authorization metadata however fits you best.
+
+To enable it create a new initializer called `config/initializers/decidim_direct_verifications.rb` with the following contents
+
+```rb
+Rails.application.config.direct_verifications_parser = :metadata
+```
+
 ## Installation
 
 Add this line to your application's Gemfile:

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

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+module Decidim
+  module DirectVerifications
+    module Verification
+      # Abstract class all concrete parsers should inherit from. They are expected to implement
+      # #header, #lines, and #parse_data methods.
+      class BaseParser
+        EMAIL_REGEXP = /([A-Z0-9+._-]+@[A-Z0-9._-]+\.[A-Z0-9_-]+)\b/i.freeze
+
+        def initialize(txt)
+          @txt = txt
+          @emails = {}
+        end
+
+        def to_h
+          lines.each do |line|
+            EMAIL_REGEXP.match(line) do |match|
+              email = normalize(match[0])
+              emails[email] = parse_data(email, line, header)
+            end
+          end
+
+          emails
+        end
+
+        private
+
+        attr_reader :txt, :emails
+
+        def normalize(value)
+          value.to_s.downcase
+        end
+      end
+    end
+  end
+end

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

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+require "csv"
+
+module Decidim
+  module DirectVerifications
+    module Verification
+      class MetadataParser < BaseParser
+        def header
+          @header ||= begin
+                        header_row = lines[0].chomp
+                        column_names = tokenize(header_row)
+                        column_names.map(&:to_sym).map(&:downcase)
+                      end
+        end
+
+        def lines
+          @lines ||= StringIO.new(txt).readlines
+        end
+
+        def parse_data(email, line, header)
+          tokens = tokenize(line)
+
+          hash = {}
+          header.each_with_index do |column, index|
+            value = tokens[index].strip
+            next if value.include?(email)
+
+            hash[column] = value
+          end
+          hash
+        end
+
+        private
+
+        def tokenize(line)
+          CSV.parse(line)[0]
+        end
+      end
+    end
+  end
+end

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

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+module Decidim
+  module DirectVerifications
+    module Verification
+      class NameParser < BaseParser
+        LINE_DELIMITER = /[\r\n;,]/.freeze
+        NON_ALPHA_CHARS = /[^[:print:]]|[\"\$\<\>\|\\]/.freeze
+
+        def header
+          nil
+        end
+
+        def lines
+          txt.split(LINE_DELIMITER)
+        end
+
+        def parse_data(email, line, _header)
+          name = parse_name(email, line)
+          name = strip_non_alpha_chars(name)
+          name.presence || fallback_name(email)
+        end
+
+        private
+
+        def strip_non_alpha_chars(str)
+          (str.presence || "").gsub(NON_ALPHA_CHARS, "").strip
+        end
+
+        def parse_name(email, line)
+          line.split(email).first
+        end
+
+        def fallback_name(email)
+          email.split("@").first
+        end
+      end
+    end
+  end
+end

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

@@ -20,11 +20,14 @@ def create
 
             @userlist = params[:userlist]
             @workflows = workflows
+
             processor = UserProcessor.new(current_organization, current_user)
-            processor.emails = extract_emails_to_hash @userlist
+            processor.emails = parser_class.new(@userlist).to_h
             processor.authorization_handler = authorization_handler(params[:authorization_handler])
+
             stats = UserStats.new(current_organization)
             stats.authorization_handler = processor.authorization_handler
+
             if params[:register]
               processor.register_users
               flash[:warning] = t(".registered", count: processor.emails.count,
@@ -52,21 +55,18 @@ def create
                                         registered: stats.registered)
               render(action: :index) && return
             end
+
             redirect_to direct_verifications_path
           end
 
           private
 
-          def extract_emails_to_hash(txt)
-            reg = /([A-Z0-9+._-]+@[A-Z0-9._-]+\.[A-Z0-9_-]+)\b/i
-            emails = {}
-            txt.split(/[\r\n;,]/).each do |line|
-              reg.match line do |m|
-                n = line.split(m[0]).first
-                emails[m[0]] = (n.presence || "").gsub(/[^[:print:]]|[\"\$\<\>\|\\]/, "").strip
-              end
+          def parser_class
+            if Rails.configuration.direct_verifications_parser == :metadata
+              MetadataParser
+            else
+              NameParser
             end
-            emails
           end
 
           def authorization_handler(authorization_handler)

lib/decidim/direct_verifications/tests/verification_controller_examples.rb

@@ -2,19 +2,23 @@
 
 shared_examples_for "checking users" do |params|
   context "when check without mails" do
+    before { params[:userlist] = "" }
+
     it "renders the index with info message" do
-      params[:userlist] = ""
       post :create, params: params
+
       expect(flash[:info]).not_to be_empty
       expect(flash[:info]).to include("0 users detected")
       expect(subject).to render_template("decidim/direct_verifications/verification/admin/direct_verifications/index")
     end
   end
 
   context "when check with mails" do
+    before { params[:userlist] = "[email protected]" }
+
     it "renders the index with info message" do
-      params[:userlist] = "[email protected]"
       post :create, params: params
+
       expect(flash[:info]).not_to be_empty
       expect(flash[:info]).to include("1 users detected")
       expect(subject).to render_template("decidim/direct_verifications/verification/admin/direct_verifications/index")
@@ -26,6 +30,7 @@
   context "when send valid emails" do
     it "creates warning message" do
       post :create, params: params
+
       expect(flash[:warning]).not_to be_empty
       expect(flash[:warning]).to include("1 detected")
       expect(flash[:warning]).to include("0 errors")
@@ -39,6 +44,7 @@
   context "when send valid emails" do
     it "creates notice message" do
       post :create, params: params
+
       expect(flash[:notice]).not_to be_empty
       expect(flash[:notice]).to include("1 detected")
       expect(flash[:notice]).to include("0 errors")
@@ -50,14 +56,18 @@
 
 shared_examples_for "revoking users" do |params|
   context "when send valid emails" do
-    it "creates notice message" do
+    before do
       create(
         :authorization,
         :granted,
         name: verification_type,
         user: authorized_user
       )
+    end
+
+    it "creates notice message" do
       post :create, params: params
+
       expect(flash[:notice]).not_to be_empty
       expect(flash[:notice]).to include("1 detected")
       expect(flash[:notice]).to include("0 errors")

lib/decidim/direct_verifications/user_processor.rb

@@ -12,17 +12,19 @@ def initialize(organization, current_user)
         @emails = {}
       end
 
-      attr_reader :organization, :current_user, :errors, :processed, :emails
-      attr_accessor :authorization_handler
-
-      def emails=(email_list)
-        @emails = email_list.map { |k, v| [k.to_s.downcase, v.presence || k.split("@").first] }.to_h
-      end
+      attr_reader :organization, :current_user, :errors, :processed
+      attr_accessor :authorization_handler, :emails
 
       def register_users
-        @emails.each do |email, name|
+        emails.each do |email, data|
           next if find_user(email)
 
+          name = if data.is_a?(Hash)
+                   data[:name]
+                 else
+                   data
+                 end
+
           form = register_form(email, name)
           begin
             InviteUser.call(form) do
@@ -41,9 +43,11 @@ def register_users
       end
 
       def authorize_users
-        @emails.each do |email, _name|
+        emails.each do |email, data|
           if (u = find_user(email))
             auth = authorization(u)
+            auth.metadata = data
+
             next unless !auth.granted? || auth.expired?
 
             Verification::ConfirmUserAuthorization.call(auth, authorize_form(u)) do
@@ -61,7 +65,7 @@ def authorize_users
       end
 
       def revoke_users
-        @emails.each do |email, _name|
+        emails.each do |email, _name|
           if (u = find_user(email))
             auth = authorization(u)
             next unless auth.granted?
@@ -87,14 +91,18 @@ def find_user(email)
       end
 
       def register_form(email, name)
-        OpenStruct.new(name: name.presence || email.split("@").first,
+        OpenStruct.new(name: name.presence || fallback_name(email),
                        email: email.downcase,
                        organization: organization,
                        admin: false,
                        invited_by: current_user,
                        invitation_instructions: "direct_invite")
       end
 
+      def fallback_name(email)
+        email.split("@").first
+      end
+
       def authorization(user)
         Authorization.find_or_initialize_by(
           user: user,

lib/decidim/direct_verifications/verification/engine.rb

@@ -6,6 +6,8 @@ module Verification
       class Engine < ::Rails::Engine
         isolate_namespace Decidim::DirectVerifications::Verification
 
+        config.direct_verifications_parser = :name
+
         paths["db/migrate"] = nil
         paths["lib/tasks"] = nil