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.

Sign up for GitHub

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
Merged

[WIP] Convert image links to shortened URLs #546

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
93e3a86
tinyurl
tomurb Jul 10, 2018
1be6bc6
forgot to add in last
tomurb Jul 10, 2018
8facb79
shorten_url service, added columns
tomurb Jul 13, 2018
2c5e3e5
Merge branch 'develop' into convert-images-links-to-shortened-urls#15…
tomurb Jul 13, 2018
eb3c901
Merge branch 'develop' into convert-images-links-to-shortened-urls#15…
tomurb Jul 19, 2018
d43264a
fixed ShortenUrl, company and user #short...url methods
tomurb Jul 19, 2018
abe568f
fixes in models, extended error logs
tomurb Jul 21, 2018
0185302
ShortenUrl and spec fixes
tomurb Jul 21, 2018
12d8de9
cognitive complexity
tomurb Jul 21, 2018
f38808c
Merge branch 'convert-images-links-to-shortened-urls#157840742' of ht…
tomurb Jul 21, 2018
d21f70e
back to the helpers
tomurb Jul 22, 2018
99b7c21
just a little change
tomurb Jul 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion app/controllers/users_controller.rb
View file
Open in desktop
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
10 changes: 10 additions & 0 deletions app/helpers/users_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
module UsersHelper

require 'tinyurl'

def most_recent_login_time user
user.current_sign_in_at.blank? ? user.last_sign_in_at : user.current_sign_in_at
end
Expand Down Expand Up @@ -41,4 +43,12 @@ def user_has_open_application(user)
user.shf_application.state.to_sym.in?([:accepted, :rejected]) ? nil : t('yes')
end

def short_proof_of_membership_url(user)
TinyURL.short(proof_of_membership_url(user))
end

def short_company_h_brand_url(user, company_id)
TinyURL.short(company_h_brand_url(user, company_id: company_id))
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two methods should be Company instance methods (hence, defined in the model file).

These methods should first determine if we already have a shortened URL for the link - if so, just return that. If not, call the service class and save the result if not nil.

Finally, if we don't have a shortened URL for the link (because it's not in the DB and the service call fails), then just return the unshortened URL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Consistent with above comment, we need a migration to add the shortened URL's to the DB.


end
6 changes: 6 additions & 0 deletions app/services/tinyurl.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class TinyURL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Recommend changing the name of this class to something generic, such as ShortenUrl. This is in case we need to change to another shortening service in the future.

def self.short(url)
HTTParty.get("http://tinyurl.com/api-create.php?url=#{url}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method should rescue any exception(s) that might occur, write something to the log, and return nil.

It looks like we can just rescue HTTParty::Error, since all the exceptions inherit from that: https://github.com/jnunemaker/httparty/blob/master/lib/httparty/exceptions.rb

Copy link
Collaborator

Choose a reason for hiding this comment

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

The "log" that I refer to should be specific to the URL shortening service.

The log should only contain exceptions - no need to log successful execution.

For an example of logging exception, you could look at log_hips_activity method in payments_controller.rb. We use our ActivityLogger class for this type of logging.

end
end

2 changes: 1 addition & 1 deletion app/views/users/_company_h_brand.html.haml
View file
Open in desktop
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)
= :hort_company_h_brand_url(user, company.id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be short_company_h_brand_url( .... )


:javascript
window.onload = function(){
Expand Down
4 changes: 2 additions & 2 deletions app/views/users/_proof_of_membership.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@
height_id: 'pom-height')
%br
%br
= t('users.show.use_this_image_link_html')
= proof_of_membership_url(user)
= t('users.show.use_this_image_link_htmlsers.show.use_this_image_link_html')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be:

= t('users.show.use_this_image_link_html')

= short_proof_of_membership_url(user)

:javascript
window.onload = function(){
Expand Down