Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Convert image links to shortened URLs #546

Merged
merged 12 commits into from
Jul 23, 2018
1 change: 0 additions & 1 deletion app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -131,5 +131,4 @@ def allow_iframe_request
# cannot-display-my-rails-4-app-in-iframe-even-if-x-frame-options-is-allowall
end


end
15 changes: 15 additions & 0 deletions app/models/company.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ class Company < ApplicationRecord

include Dinkurs::Errors

include Rails.application.routes.url_helpers

before_destroy :destroy_checks

validates_presence_of :company_number
Expand Down Expand Up @@ -177,6 +179,19 @@ def se_mailing_csv_str
AddressExporter.se_mailing_csv_str( main_address )
end

def get_or_create_short_h_brand_url(user_id=0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We should not use a user_id argument, since that would imply that we would be OK with calling this method with an actual user.id - which we do not want to happen since we would then be creating different URLs for the same company for different users - which is not what we want to do.

  2. The caller of this method should not care whether or not we have to create the shortened URL or just retrieve it from the DB. So, maybe the name should just be get_short_h_brand_url.

found = self.short_h_brand_url
return found unless found.nil?
url = company_h_brand_url(user_id, company_id: self.id)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass 0 as the user_id.

Note: this is an argument for moving the controller action for accessing the h-brand image from the users controller to the companies controller. When I first created that action I was not sure whether the company's h-brand image would contain anything specific to the user - which, of course, it doesn't. I'll create a story to make that change (this note does not affect this PR, however).

short_url = ShortenUrl.short(url)
unless short_url.nil?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be simplified as

if short_url

self.update_attribute(:short_h_brand_url, short_url)
short_url
else
url
end
end


private

Expand Down
15 changes: 15 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
class User < ApplicationRecord
include PaymentUtility

include Rails.application.routes.url_helpers

# Include default devise modules. Others available are:
# :confirmable, :lockable, :timeoutable and :omniauthable
devise :database_authenticatable, :registerable,
Expand Down Expand Up @@ -117,6 +119,19 @@ def grant_membership(send_email: true)
Arel.sql("lpad(membership_number, 20, '0')")
end

def get_or_create_short_proof_of_membership_url
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comments as above.

found = self.short_proof_of_membership_url
return found unless found.nil?
url = proof_of_membership_url(self.id)
short_url = ShortenUrl.short(url)
unless short_url.nil?
self.update_attribute(:short_proof_of_membership_url, short_url)
short_url
else
url
end
end

private

def issue_membership_number
Expand Down
17 changes: 17 additions & 0 deletions app/services/shorten_url.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
class ShortenUrl

SHORTEN_URL_LOG = 'log/tinyurl.log'
SUCCESS_CODES = [200, 201, 202].freeze

def self.short(url)
response = HTTParty.get("http://tinyurl.com/api-create.php?url=#{url}")
raise (response) if response.match?(/error/i) || !SUCCESS_CODES.include?(response.code)
response
rescue Exception => error
ActivityLogger.open(SHORTEN_URL_LOG, 'TINYURL_API', 'shortening url', false) do |log|
log.record('error', error.message)
end
nil
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're raising a Runtime error and then rescuing any exception.

  1. I'd rather raise an HTTParty::Error since that is what we have - an error code and/or body message has been returned that indicates an error.

  2. In general, one should not rescue very broad categories of exceptions. Otherwise, we could be handling an exception that has nothing to do with this method (happening down in the stack).

  3. I'd like to see more information logged - that's so important in helping us figure out problems in production.

I altered this code to:

def self.short(url)
    response = HTTParty.get("http://tinyurl.com/api-create.php?url=#{url}")
    raise HTTParty::Error if response.match?(/error/i) || !SUCCESS_CODES.include?(response.code)
    response
rescue HTTParty::Error => error
    ActivityLogger.open(SHORTEN_URL_LOG, 'TINYURL_API', 'shortening url', false) do |log|
      log.record('error', "Exception: #{error.message}")
      log.record('error', "Attempted URL: #{url}")
      log.record('error', "Response body: #{response.body}")
      log.record('error', "HTTP code: #{response.code}")
    end
    nil
end

I then called the method with: ShortenUrl.short('') and ShortenUrl.short('/') and this what I got in the log:

# Logfile created on 2018-07-20 07:42:14 -0400 by logger.rb/61378
[TINYURL_API] [shortening url] [info] Started at 2018-07-20 11:42:14 UTC
[TINYURL_API] [shortening url] [error] Exception: HTTParty::Error
[TINYURL_API] [shortening url] [error] Attempted URL: 
[TINYURL_API] [shortening url] [error] Response body: Error
[TINYURL_API] [shortening url] [error] HTTP code: 400
[TINYURL_API] [shortening url] [info] Finished at 2018-07-20 11:42:14 UTC.
[TINYURL_API] [shortening url] [info] Duration: 0.0 seconds.

[TINYURL_API] [shortening url] [info] Started at 2018-07-20 11:42:44 UTC
[TINYURL_API] [shortening url] [error] Exception: HTTParty::Error
[TINYURL_API] [shortening url] [error] Attempted URL: /
[TINYURL_API] [shortening url] [error] Response body: ERROR
[TINYURL_API] [shortening url] [error] HTTP code: 200
[TINYURL_API] [shortening url] [info] Finished at 2018-07-20 11:42:44 UTC.
[TINYURL_API] [shortening url] [info] Duration: 0.0 seconds.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this approach allows us to capture more information for HTTParty-specific errors.

For other exceptions, we could follow this with a second rescue clause, picking up other exceptions such as the one you have now.

That way we rescue all exceptions, but we maximize the information we can get from HTTParty exceptions.

end

2 changes: 1 addition & 1 deletion app/views/users/_company_h_brand.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
%br
%br
= t('users.show.use_this_image_link_html')
= company_h_brand_url(user, company_id: company.id)
= company.get_or_create_short_h_brand_url

:javascript
window.onload = function(){
Expand Down
2 changes: 1 addition & 1 deletion app/views/users/_proof_of_membership.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
%br
%br
= t('users.show.use_this_image_link_html')
= proof_of_membership_url(user)
= user.get_or_create_short_proof_of_membership_url

:javascript
window.onload = function(){
Expand Down
1 change: 1 addition & 0 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ class Application < Rails::Application

# Default for development and testing. production.rb overrides this
config.action_mailer.default_url_options = { host: 'localhost', port: '3000' }
routes.default_url_options = { host: 'localhost', port: '3000' }

# Ensure this is false by default (to be secure).
# Only change in development or test environments where really needed
Expand Down
10 changes: 10 additions & 0 deletions config/environments/production.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,16 @@
#
###

###
#
# Routes
#

# Needed to create shortened urls in the ShortLinksContainer model.
routes.default_url_options = { host: ENV['DEFAULT_HOST'] }

#
###

# Enable locale fallbacks for I18n (makes lookups for any locale fall back to
# the I18n.default_locale when a translation cannot be found).
Expand Down
1 change: 1 addition & 0 deletions config/initializers/shorten_url.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
TINYURL_LOG = 'log/shorten_url.log'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not used anymore(?)

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddShortHBrandUrlToCompanies < ActiveRecord::Migration[5.1]
def change
add_column :companies, :short_h_brand_url, :string
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddShortProofOfMembershipUrlToUsers < ActiveRecord::Migration[5.1]
def change
add_column :users, :short_proof_of_membership_url, :string
end
end
8 changes: 4 additions & 4 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20180428103625) do
ActiveRecord::Schema.define(version: 20180719021503) do

# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
Expand Down Expand Up @@ -94,6 +94,8 @@
t.datetime "updated_at", null: false
t.text "description"
t.string "dinkurs_company_id"
t.boolean "show_dinkurs_events"
t.string "short_h_brand_url"
t.index ["company_number"], name: "index_companies_on_company_number", unique: true
end

Expand Down Expand Up @@ -229,6 +231,7 @@
t.string "member_photo_content_type"
t.integer "member_photo_file_size"
t.datetime "member_photo_updated_at"
t.string "short_proof_of_membership_url"
t.index ["email"], name: "index_users_on_email", unique: true
t.index ["membership_number"], name: "index_users_on_membership_number", unique: true
t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true
Expand All @@ -240,9 +243,6 @@
add_foreign_key "company_applications", "companies"
add_foreign_key "company_applications", "shf_applications"
add_foreign_key "events", "companies"
add_foreign_key "company_applications", "companies"
add_foreign_key "company_applications", "shf_applications"
add_foreign_key "events", "companies"
add_foreign_key "payments", "companies"
add_foreign_key "payments", "users"
add_foreign_key "shf_applications", "member_app_waiting_reasons", column: "member_app_waiting_reasons_id"
Expand Down
10 changes: 7 additions & 3 deletions db/structure.sql
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,8 @@ CREATE TABLE public.companies (
updated_at timestamp without time zone NOT NULL,
description text,
dinkurs_company_id character varying,
show_dinkurs_events boolean
show_dinkurs_events boolean,
short_h_brand_url character varying
);


Expand Down Expand Up @@ -700,7 +701,8 @@ CREATE TABLE public.users (
member_photo_file_name character varying,
member_photo_content_type character varying,
member_photo_file_size integer,
member_photo_updated_at timestamp without time zone
member_photo_updated_at timestamp without time zone,
short_proof_of_membership_url character varying
);


Expand Down Expand Up @@ -1318,6 +1320,8 @@ INSERT INTO "schema_migrations" (version) VALUES
('20180326103433'),
('20180328105100'),
('20180428103625'),
('20180624155644');
('20180624155644'),
('20180717043851'),
('20180719021503');


1 change: 1 addition & 0 deletions spec/factories/companies.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
phone_number '123123123'
email 'thiscompany@example.com'
website 'http://www.example.com'
short_h_brand_url nil

transient do
num_addresses 1
Expand Down
2 changes: 2 additions & 0 deletions spec/factories/users.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
password 'my_password'
admin false
member false
short_proof_of_membership_url nil
member_photo do
File.new("#{Rails.root}/spec/fixtures/member_photos/photo_unavailable.png")
end
Expand Down Expand Up @@ -44,4 +45,5 @@
end
end


end
28 changes: 28 additions & 0 deletions spec/models/company_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@

RSpec.describe Company, type: :model do

include Rails.application.routes.url_helpers

let(:with_short_h_brand_url) do
create(:company, short_h_brand_url: 'http://www.tinyurl.com/hbrand')
end

let(:no_name) do
create(:company, name: '', company_number: '2120000142')
end
Expand Down Expand Up @@ -94,6 +100,7 @@
it { is_expected.to have_db_column :description }
it { is_expected.to have_db_column :dinkurs_company_id }
it { is_expected.to have_db_column :show_dinkurs_events }
it { is_expected.to have_db_column :short_h_brand_url }
end

describe 'Validations' do
Expand Down Expand Up @@ -697,4 +704,25 @@
end
end

describe '#get_or_create_short_h_brand_url' do
context 'there is already a shortened url in the table' do
it 'returns shortened url' do
expect(with_short_h_brand_url.get_or_create_short_h_brand_url).to eq('http://www.tinyurl.com/hbrand')
end
end
context 'there is no shortened url in the table and ShortenUrl.short is called' do
it 'saves the result if the result is not nil and returns shortened url' do
url = company_h_brand_url(0, company_id: complete_co.id)
allow(ShortenUrl).to receive(:short).with(url).and_return('http://tinyurl.com/hbrand2')
expect(complete_co.get_or_create_short_h_brand_url).to eq(ShortenUrl.short(url))
expect(complete_co.short_h_brand_url).to eq(ShortenUrl.short(url))
end
it 'does not save anything if the result is nil and returns unshortened url' do
url = company_h_brand_url(0, company_id: complete_co.id)
allow(ShortenUrl).to receive(:short).with(url).and_return(nil)
expect(complete_co.get_or_create_short_h_brand_url).to eq(url)
expect(complete_co.short_h_brand_url).to eq(nil)
end
end
end
end
29 changes: 29 additions & 0 deletions spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,17 @@
require 'email_spec/rspec'

RSpec.describe User, type: :model do

include Rails.application.routes.url_helpers

let(:user) { create(:user) }
let(:user_with_app) { create(:user_with_membership_app) }
let(:member) { create(:member_with_membership_app) }

let(:with_short_proof_of_membership_url) do
create(:user, short_proof_of_membership_url: 'http://www.tinyurl.com/proofofmembership')
end

let(:application) do
create(:shf_application, user: user, state: :accepted)
end
Expand Down Expand Up @@ -535,4 +542,26 @@
end
end
end

describe '#get_or_create_short_proof_of_membership_url' do
context 'there is already a shortened url in the table' do
it 'returns shortened url' do
expect(with_short_proof_of_membership_url.get_or_create_short_proof_of_membership_url).to eq('http://www.tinyurl.com/proofofmembership')
end
end
context 'there is no shortened url in the table and ShortenUrl.short is called' do
it 'saves the result if the result is not nil and returns shortened url' do
url = proof_of_membership_url(user.id)
allow(ShortenUrl).to receive(:short).with(url).and_return('http://tinyurl.com/proofofmembership2')
expect(user.get_or_create_short_proof_of_membership_url).to eq(ShortenUrl.short(url))
expect(user.short_proof_of_membership_url).to eq(ShortenUrl.short(url))
end
it 'does not save anything if the result is nil and returns unshortened url' do
url = proof_of_membership_url(user.id)
allow(ShortenUrl).to receive(:short).with(url).and_return(nil)
expect(user.get_or_create_short_proof_of_membership_url).to eq(url)
expect(user.short_proof_of_membership_url).to eq(nil)
end
end
end
end
19 changes: 19 additions & 0 deletions spec/services/shorten_url_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
require 'rails_helper'

describe ShortenUrl do
describe '.short' do
it 'creates shortened link' do
VCR.use_cassette('shorten_url/short') do
shortened_url = ShortenUrl.short 'http://sverigeshundforetagare.se/anvandare/2/proof_of_membership'
expect(shortened_url).to match 'tinyurl.com'
end
end
it 'if the service raises an error, returns nil and writes to the log' do
VCR.use_cassette('shorten_url/short') do
expect(ActivityLogger).to receive(:open)
shortened_url = ShortenUrl.short '/'
expect(shortened_url).to eq nil
end
end
end
end
43 changes: 43 additions & 0 deletions spec/vcr_cassettes/shorten_url/short.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.