-
Notifications
You must be signed in to change notification settings - Fork 37
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
Changes from 6 commits
93e3a86
1be6bc6
8facb79
2c5e3e5
eb3c901
d43264a
abe568f
0185302
12d8de9
f38808c
d21f70e
99b7c21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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) | ||
found = self.short_h_brand_url | ||
return found unless found.nil? | ||
url = company_h_brand_url(user_id, company_id: self.id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pass Note: this is an argument for moving the controller action for accessing the h-brand image from the |
||
short_url = ShortenUrl.short(url) | ||
unless short_url.nil? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be simplified as
|
||
self.update_attribute(:short_h_brand_url, short_url) | ||
short_url | ||
else | ||
url | ||
end | ||
end | ||
|
||
|
||
private | ||
|
||
|
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, | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're raising a
I altered this code to:
I then called the method with:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
TINYURL_LOG = 'log/shorten_url.log' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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 |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not use a
user_id
argument, since that would imply that we would be OK with calling this method with an actualuser.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.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
.