hyperoslo / cellular Goto Github PK

By default, the Sidekiq job uses the cellular queue which mean you have to add it to your Sidekiq configuration, like:

$ sidekiq -q default -q cellular

or

# config/sidekiq.yml
:queues:
  - default
  - cellular

The problem with this config is that is accidentally implying importance. This means that the default queue has more priority than the cellular queue.

If you want to use the weight for both queues, you can do:

$ sidekiq -q default,1 -q cellular,1

or

# config/sidekiq.yml
:queues:
  - [default, 1]
  - [cellular, 1]

Another fix is to use the default queue instead of cellular, and provide a way to override the default queue name. This is not backward compatible, but it plays good with common production setups:

# config/sidekiq.yml
:queues:
  - [high, 4]
  - [default, 2]
  - [low, 1]

Right now, I'm using this config:

# config/sidekiq.yml
:queues:
  - [high, 4]
  - [default, 2]
  - [mailers, 2]
  - [low, 1]

I'm not using the cellular queue but it's there just in case a developer uses deliver_async without specifying a queue. I would like to remove it, so Sidekiq doesn't have to poll an unused queue.

References:

Sidekiq Advanced Options

We should rename Cellular::Backends::Log to Cellular::Backends::Test and make the deliveries array available on it rather than the Cellular module.
#9

It should be configurable whether Cellular pushes messages onto deliveries so you can use it with any backend.

Is this expected?

https://github.com/hyperoslo/cellular/blob/master/lib/cellular/backends/twilio.rb#L23

We should use country_code instead of country for consistency.

• Remove unneeded dependencies (e.g. having savon when only using the Twilio backend).
• Better integration with ActiveJob. Right now it tries to load Sidekiq even when it uses ActiveJob as an interface.
• Better test helper.
• Check what backends should we support.
• #26 Don't mutate given options hash.
• #28 Be able to configure default queue name.
• #2 Check if SMS was delivered
• Add support for Ruby 2.4 - Currently there are some failing tests.
• Add support for Rails 5.0+
• Remove Rails dependency. It should be possible to use it outside Rails.

Cellular::Backend::Logs currently logs to STDOUT, whereas ActionMailer logs to log/development.log.

This causes SMS to appear in the output from rails server, whereas e-mail appear in log/development.log.

We should probably do that, too.

The current deliver-method completes gracefully regardless of delivery status.

Under certain circumstances it might be useful to have Cellular raise an exception in case of a failed delivery: SMS::deliver!

Proposal to deprecate Cellular::Backends and introduce:

class Cellular::Backend
  def deliver(...)
    raise NotImplementedError  
  end
end

class Cellular::Backend::HyperSMS < Cellular::Backend; end

For backwards compatibility we could point Cellular::Backends to Cellular::Backend.

From Sendega's API documentation:

Any ideas on implementation? We could theoretically introduce Cellular::SMS#recipients and then have the backends handle it accordingly.

At present an SMS is marked as delivered regardless of whether a backend ran into issues processing it.

We should set up a uniform delivery object, for example:

class Cellular::Delivery
  attr_reader :sms, :receipt

  delegate :success?, :failure?, to: :receipt
end

class Cellular::Receipt
  attr_reader :status, :message, :raw

  def success?; end
  def failure?; end
end

Cellular::SMS#deliver would receive a Cellular::Receipt from a backend, construct a delivery instance and be able to use this receipt to set delivery status as well as return the delivery back to the caller for further processing if required.

For testing purposes, this delivery object (as well as the actual SMS and the backend receipt) will be available in testing backend, see #10.

Sending texts through Sendega doesn't work by default, unless we specify the default SSL version for Excon to be TLSv1:

Excon.defaults[:ssl_version] = :TLSv1

While we could specify this in the application, it'd be better to set it specifically when sending through Sendega, as setting it globally in the application might cause an issue elsewhere.

Savon defaults to spamming you with logs. We should default to not doing that.