survival/donation-system

:gem: Gem containing Survival's donation system logic

donation-system's Issues

Integration tests

See our contributing guides.

Stripe is more or less stable now. The tests in the resource creator have just one success test and the rest of tests are testing what we do when something fails. For the success test and for the failure tests we are doing real requests that are then replayed by VCR.

At the moment we may be in a position where we can avoid those cassettes completely by adding this to each of the parameter tests in resource_creator_spec.rb, right before the call to expect_creation_to_fail_with():

allow(resource).to receive(:create).with(fields).and_raise(Stripe::EXCEPTION_NAME_HERE)

Then we should have a spec/donation_system/integration_spec.rb file in the top level that creates one donation of each type (Stripe one-off, Stripe recurring, PayPal one-off, PayPal recurring, etc.) passing an instance of RawRequestData with the relevant fields for each donation to the Payment object. These tests will then do real requests so we would use VCR or webmock for those tests only (VCR and webmock fixtures are a pain to maintain and add a lot of noise to git, so better to keep them either small or out of the git history completely as we are doing at the moment).

Update ruby version to 2.5.0

See our contributing guides.

Our current Ruby version is 2.4.1. Switch the Ruby version to 2.5.0

To get started

• Claim this issue by clicking the "Assignees" cog to the right and assigning yourself
• Check out our contributing guides

Steps for this issue:

• Install Ruby version 2.5.0 on your local system. How to do this depends on the Ruby version manager that you use. (this is an example for the rbenv manager)
• Set your Ruby version to 2.5.0. This again depends on your Ruby version manager.
• Change the Ruby version in the Gemfile of this project.
• Install the bundler gem for that version: gem install bundler.
• Run bundle install and check that the tests and the app work correctly.
• You may or may not have to upgrade some gems. If you have to, do it one at a time, like this:
  • run bundle update GEMNAME
  • Run the tests (check README), they should be green
  • Run the app (check README), it should work
  • commit!
• If everything works, update the Ruby version in the .travis.yml file and add - gem update --system to the list of commands below before_install:
• Update the Ruby version in the .rubocop.yml file
• When you are finished, push your branch and submit a PR with your changes.

Calculate the web payment number

See our contributing guides.

The old site calls payment.number, which is defined in the Payment model as:

  def generate_payment_number                
    return unless number.blank?
    record = true
    while record
      random = "P#{Array.new(9){rand(9)}.join}"                                        
      record = Payment.find(:first, :conditions => ["number = ?", random])
    end          
    self.number = random
  end

  def generate_mandate_number                
    return unless number.blank?
    record = true
    while record
      random = "#{number_prefix}#{Array.new(9){rand(9)}.join}"                                        
      record = Mandate.find(:first, :conditions => ["number = ?", random])
    end          
    self.number = random
  end   

Credit card mandate:

  def number_prefix
    "MC"
  end

Direct debit mandate:

  def number_prefix
    "MD"
  end

Generic direct debit mandate:

  def number_prefix
    "MG"
  end

Paypal mandate:

  def number_prefix
    "MP"
  end

Spendenbank direct debit mandate:

  def number_prefix
    "MS"
  end

Investigate why it needs this format and if there is a better way to implement this code.
Implement this in the StripeWrapper::FieldsGenerator class

Implement Salesforce users

The old site was using an OwnerId field in the Contacts table (supporters table) which is a Salesforce user (SalesforceUser.current_user.Id). In this gem, it should go in the fields returned by DonationFieldsGenerator.

In the old site, vendor/plugins/salesforce_integration/app/models/salesforce_user.rb has a definition of current_user which is defined as:

class SalesforceUser
  def self.current_user
    salesforce_username = ENV['SALESFORCE_USERNAME']
    SalesforceModel::User.find_by_Username(salesforce_username)
  end
end

Investigate field values' truncation

See our contributing guides.

The old site is truncating some fields, for example:

MailingCity => truncate(last_complete_address(supporter).try(:city), 40)

Investigate if this is a restriction in Salesforce and if it is, see if there is a better way to calculate this truncations.

Implement supporter name split

See our contributing guides.

The field LastName is marked as a required field in our Salesforce setup upstream. We didn't want to implement all at once so we are passing the full name for now until we implement this feature.

The task consists of splitting the name received from the request in data.name and saving it in the relevant fields in the Salesforce supporter table. These are the relevant fields defined there:

FIELD NAME | FIELD LABEL
-------------------------
LastName   | Last Name
FirstName  | First Name
Name       | Full Name

A good place to start is where the fields to be passed to the supporter table are generated.

We don't have to maintain this name-split logic, there are people out there who have solved this problem already, for example namae, whose README I found impressive.

Implement logging errors

See our contributing guides.

At the moment of writing this, the #attempt method in the Payment class looks like this:

def attempt
  result = PaymentGateway.charge(request_data)
  return result.errors unless result.okay?
  ThankYouMailer.send_email(request_data.email, request_data.name)
  Salesforce::Database.add_donation(
    DonationData.new(request_data, result.item)
  )
end

The problem with this is that not only payment errors will be shown to the user, but also errors from Salesforce that are not relevant to the user.

Also, with this we only have access to the custom error codes (symbols).

Proposed solution

Implement an error logging system that saves errors as a struct of the type:

DonationSystem::Error = Struct.new(code, error)

where code represents the symbol we have been using, and error is whatever error occurred (the different payment gateways and Salesforce all will produce errors in different formats)

With this in place, instead of tracking errors with just the custom code as we are doing now:

rescue SomeException
  errors << :custom_error_symbol
end

we can use the error struct:

require_relative 'path/to/error'
rescue SomeException => error
  errors << Error.new(:custom_error_symbol, error)
end

The idea is that, for payment errors that are relevant to the user, we will still return just the symbols array to the consumer of the gem, which can then be mapped to actual error messages in different languages:

def attempt
  result = PaymentGateway.charge(request_data)
  return error_codes(result.errors) unless result.okay?
# ...
end

private

def error_codes(errors)
  # Log errors.map(&:error) in some way, for developers
  errors.map(&:code)
end

And for the actual errors that are relevant to developers, we can log them in some way. For example, we could write them to a log file.

Then, no matter if there were errors in Salesforce or not, as long as the payment went through successfully, we will still send an empty array:

def attempt
  result = PaymentGateway.charge(request_data)
  return error_codes(result.errors) unless result.okay?
  ThankYouMailer.send_email(request_data.email, request_data.name)
  errors = Salesforce::Database.add_donation(
    DonationData.new(request_data, result.item)
  )
  log_salesforce_errors(errors)
end

private

def log_salesforce_errors(errors)
  # Log errors.map(&:error) in some way, for developers
  []
end

Set up Heroku to run the app

• Setup an account in Heroku to run the donations system.
• Figure out staging/production and credentials

In the future, this repo could be a gem consumed by some very light weight framework with simple views.

Setup environment variables

• Set up free private repository in Gilab for the environment variables
• Clone it inside of this repo and gitignore it

This way, we avoid having to copy paste things manually every time we change the credentials.
Whenever we have to modify credentials, we enter that folder, commit and push, then go back to the repo.

Merge validators into a single composite

See our contributing guides.

There are now several validator classes that share validations, and these validations are done in a similar way.

Remove this repetition by moving all validator methods to its own file, and then replace the validator classes with a single composite class that picks the validations it needs.

Then the Validation class doesn't need to be exposed and can be a private class of the composite.

For example:

require 'utils'

class Validators
  def initialize(data)
    @data = data
  end

  def validate_data
    :missing_data unless data
  end

  def validate_type
    :invalid_donation_type unless data&.type &&
                                  VALID_TYPES.values.include?(data.type)
  end

  def validate_amount
    :invalid_amount unless data&.amount && utils.number?(data.amount) &&
                           utils.number(data.amount).positive?
  end

  # ... etc.

  private

  attr_reader :data
end

and the composite class:

# ...
require_relative 'validators'

module DonationSystem
  class Validator
    def self.execute(data, list)
      new(data, list).execute
    end

    def initialize(data, list)
      @data = data
      @list = list
    end

    def execute
      validation = Validation.new
      validators = Validators.new(data)
      list.each { |item| validation << validators.send(:"validate_#{item}") }
      validation
    end

    private

    attr_reader :data, :list

    class Validation
      attr_reader :errors

      def initialize
        @errors = []
      end

      def <<(error)
        errors << error if error
      end

      def okay?
        errors.empty?
      end
    end
  end
end

Then, whenever a validator class is used, it can be replaced with this composite:

    def validation
      list = [:data, :type, :amount, :currency, :token, :email, :method]
      @validation ||= Validator.execute(data, list)
    end

It may be tempting to use Validators as an included module, but that introduces an inheritance pattern that is not needed here, as different validators will require a different list of validator methods.

Supress Paypal logger in tests

See our contributing guides.

Now that the Paypal gem is in place, when you run the tests there are a lot of INFO and DEBUG noisy messages printed to the terminal that are not very relevant (they are in production though).

Use the Paypal logger to set the logging level to nothing.

Think about moving the Salesforce validator one level up

See our contributing guides.

In the Salesforce::Database class, the wiring of our business logic is implemented:

• search supporter,
• if it doesn't exist, create it,
• then create a donation associated to the supporter.
• return the errors of these operations

However, there is also this:

      def add_donation
        return [:missing_email] unless email_present?
        # ...
      end

      # ...

      def email_present?
        data &&
          data.respond_to?(:request_data) &&
          data.request_data.respond_to?(:email) &&
          data.request_data.email
      end

The reason for this is that the email is explicitly used in the supporter search in that class.

The input data for supporters and donations is the same, so I think this would be totally unnecessary if we merged the supporter and donation validators into one validator to use there:

      def add_donation
        return validation.errors unless validation.okay?
        # ...
      end

      # ...

      def validation
        @validation ||= Validator.execute(data)
      end

We would need to split the validation part from the field generation part and merge only the code that does the validations. Then, the two supporter and donation validators would rather be two fields-generators and be used to produce fields for each.

Implement all Stripe errors

At some point come back to implement all the errors in the docs:

https://stripe.com/docs/api/curl#errors

Implement proper email validation

See our contributing guides.

The internet and the different gems out there don't seem to agree on which is the ultimate regex to validate emails:

%r{/\A[^@\s]+@([^@\s]+\.)+[^@\s]+\z/},

%r{/\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z/},

%r{/\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i},

%r{/^([\w+-.%]+@[\w-.]+\.[A-Za-z]{2,4},*[\W]*)+$/},

%r{/\w+@\w+\.{1}[a-zA-Z]{2,}/}

# ... and more...

Find a way to validate emails properly, etiher with regex or something else