[WIP] Convert image links to shortened URLs #546

tomurb · 2018-07-09T10:01:35Z

PT Story: https://www.pivotaltracker.com/story/show/157840742

Changes proposed in this pull request:

Creating shortened urls for user images

At the moment I do it only for company_h_brand_url just to keep it simpler when I don't know how can it be done for localhost.

Cucumber tests fail but I didn't work on them yet - there is 28 of them and they are independent from my changes in _company_h_brand.html.haml and users_helper.rb.

Ready for review:
@

tomurb · 2018-07-10T12:21:50Z

patmbolger

Looks like you're making changes as I review this :)

I'll just post what I have now (some or all are probably fixed now).

Will continue review with separate comments later.

patmbolger · 2018-07-10T13:19:18Z

app/services/tinyurl.rb

@@ -0,0 +1,6 @@
+class TinyURL


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.

patmbolger · 2018-07-10T13:24:59Z

app/views/users/_company_h_brand.html.haml

@@ -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)


This should be short_company_h_brand_url( .... )

patmbolger · 2018-07-10T13:28:16Z

app/views/users/_proof_of_membership.html.haml

@@ -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')


This should be:

= t('users.show.use_this_image_link_html')

patmbolger

More comments.

Please note that we'll need unit test(s) for the service class. Use VCR to mock the service response.

Also, will need tests for the 2 additional Company instance method(s) (see comments).

I'll have more comments on code as tests as we proceed, but that's enough for now.

patmbolger · 2018-07-10T13:51:13Z

app/services/tinyurl.rb

@@ -0,0 +1,6 @@
+class TinyURL
+  def self.short(url)
+    HTTParty.get("http://tinyurl.com/api-create.php?url=#{url}")


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

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.

patmbolger · 2018-07-10T13:58:13Z

app/helpers/users_helper.rb

+
+  def short_company_h_brand_url(user, company_id)
+    TinyURL.short(company_h_brand_url(user, company_id: company_id))
+  end


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.

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

…7840742 'updating'

houndci-bot · 2018-07-13T10:48:28Z

app/models/company.rb

@@ -177,6 +177,12 @@ def se_mailing_csv_str
      AddressExporter.se_mailing_csv_str( main_address )
  end

+  def get_or_create_short_h_brand_url(user)


Lint/UnusedMethodArgument: Unused method argument - user. If it's necessary, use _ or _user as an argument name to indicate that it won't be used. You can also write as get_or_create_short_h_brand_url(*) if you want the method to accept any arguments but don't care about them.

tomurb · 2018-07-13T10:52:07Z

Sorry for late response. I did not have much time this week.
That will be all for today. I'd like to know if I'm moving in the right direction.

For context: TinyURL does not send back many errors. It even doesn't check if url has a dot. I hope that's good way to test it.

tomurb · 2018-07-14T11:56:43Z

I see now how many things I did wrong. I will have to rewrite some stuff. Before that don't review anything but ShortenUrl.

patmbolger · 2018-07-14T18:09:44Z

app/services/shorten_url.rb

+
+  def self.short(url)
+    response = HTTParty.get("http://tinyurl.com/api-create.php?url=#{url}")
+    raise HTTParty::Error.new response if response.match? 'ERROR'


I'm note sure we'll ever see a response that matches that string. You should check the response.code against "success" codes, and raise the exception if it is not.

For example, I forced a failure by sending a bad URI:

response = HTTParty.get("http://tinyurl.com/api-create.ph?url=''") => "<?xml version=\"1.0\" encoding=\"iso-8859-1\"?>\n" + "<!DOCTYPE html PUBLIC \"-//W3C//DTD XHTML 1.0 Transitional//EN\"\n" + " \"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd\">\n" + "<html xmlns=\"http://www.w3.org/1999/xhtml\" xml:lang=\"en\" lang=\"en\">\n" + " <head>\n" + " <title>404 - Not Found</title>\n" + " </head>\n" + " <body>\n" + " <h1>404 - Not Found</h1>\n" + " </body>\n" + "</html>\n" [4] pry(main)> response.inspect => "#<HTTParty::Response:0x7fd4ae91e850 parsed_response=\"<?xml version=\\\"1.0\\\" encoding=\\\"iso-8859-1\\\"?>\\n<!DOCTYPE html PUBLIC \\\"-//W3C//DTD XHTML 1.0 Transitional//EN\\\"\\n \\\"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd\\\">\\n<html xmlns=\\\"http://www.w3.org/1999/xhtml\\\" xml:lang=\\\"en\\\" lang=\\\"en\\\">\\n <head>\\n <title>404 - Not Found</title>\\n </head>\\n <body>\\n <h1>404 - Not Found</h1>\\n </body>\\n</html>\\n\", @response=#<Net::HTTPNotFound 404 Not Found readbody=true>, @headers={\"date\"=>[\"Sat, 14 Jul 2018 18:03:21 GMT\"], \"content-type\"=>[\"text/html\"], \"transfer-encoding\"=>[\"chunked\"], \"connection\"=>[\"close\"], \"set-cookie\"=>[\"__cfduid=d071b4ebad4a29a20fd2ba0b0f2cd456a1531591401; expires=Sun, 14-Jul-19 18:03:21 GMT; path=/; domain=.tinyurl.com; HttpOnly\"], \"server\"=>[\"cloudflare\"], \"cf-ray\"=>[\"43a5e7d1967f2180-EWR\"]}>" [5] pry(main)> [6] pry(main)> response.code => 404 [7] pry(main)> response.message => "Not Found"

In hips_service.rb, for example, we do this:

SUCCESS_CODES = [200, 201, 202].freeze . . . parsed_response = response.parsed_response return parsed_response if SUCCESS_CODES.include?(response.code) . <handle HTTP error>

Also, you don't need the .new - the exception will be created for you when you raise.

pry(main)> response = HTTParty.get("http://tinyurl.com/api-create.php?url=''") => "http://tinyurl.com/6mfcgml"

You lost 'p' in 'php' before '?'. The two only errors I know of look like this.

[10] pry(main)> response = HTTParty.get("http://tinyurl.com/api-create.php?url=/") => "ERROR" [11] pry(main)> response.code => 200 [12] pry(main)> response = HTTParty.get("http://tinyurl.com/api-create.php?url=") => "Error" [13] pry(main)> response.code => 400

Second one I found out about a moment ago. I will change match to /error/i I guess.

OK, that will work. What I was trying to get at is the situation where the website is down and/or the request times out - in that case we won't get an "Error" from the site, but we should get a response.code other than 200.

So, maybe we should check for response matching /error/i or response.code != 200.

…7840742

patmbolger · 2018-07-19T21:45:10Z

app/models/company.rb

@@ -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)


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.

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.

patmbolger · 2018-07-19T21:49:43Z

app/models/company.rb

+  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)


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).

patmbolger · 2018-07-19T21:50:12Z

app/models/company.rb

+    return found unless found.nil?
+    url = company_h_brand_url(user_id, company_id: self.id)
+    short_url = ShortenUrl.short(url)
+    unless short_url.nil?


Could be simplified as

if short_url

patmbolger · 2018-07-19T21:50:39Z

app/models/user.rb

@@ -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


Similar comments as above.

patmbolger · 2018-07-20T11:53:12Z

app/services/shorten_url.rb

+      log.record('error', error.message)
+    end
+    nil
+  end


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

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.

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).

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.

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.

patmbolger · 2018-07-20T12:41:29Z

config/initializers/shorten_url.rb

@@ -0,0 +1 @@
+TINYURL_LOG = 'log/shorten_url.log'


This is not used anymore(?)

patmbolger · 2018-07-21T10:17:20Z

Your spec error is due to the need for a separate VCR cassette that contains the error response.

I changed the test to this:

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/ya6sa84h'
      end
    end
    it 'if the service raises an error, returns nil and writes to the log' do
      VCR.use_cassette('shorten_url/error') do
        expect(ActivityLogger).to receive(:open)
        shortened_url = ShortenUrl.short '/'
        expect(shortened_url).to eq nil
      end
    end
  end
end

That is, I specified a second cassette for the error response.

(I also specified the entire URL for the successful test - might as well since that should be the same URL every time).

BTW, these are good tests - I especially like the confirmation of opening the log file.

patmbolger · 2018-07-21T10:42:40Z

app/services/shorten_url.rb

+  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}")


One more change requested (I missed this before) - we should handle the situation that occurs if response is nil.

This is because response will be nil if HTTParty raises the exception. If we raise it, then it will not be nil.

Something like:

log.record('error', "Exception: #{error.message}") if response log.record('error', "Attempted URL: #{url}") log.record('error', "Response body: #{response.body}") log.record('error', "HTTP code: #{response.code}") else log.record('error', 'Exception raised by HTTParty') end

…tps://github.com/tmszrbn/shf-project into convert-images-links-to-shortened-urls#157840742

patmbolger · 2018-07-22T12:39:45Z

@tmszrbn - For some reason, in the test environment, the default locale is not being reset back to 'sv' before each test. And, after a specific test(*) changes the locale to 'en', the router is then adding 'en' to all other routes (which means URL's now look like " .... /en/en/ ....", and this breaks subsequent tests.

(*the test is: show-user-details.feature:141).

The tests work if line 64 in config/application.rb is changed to:

routes.default_url_options = { host: 'localhost', port: '3000', locale: 'sv' }

I think this problem is limited to the test environment. However, I'm not sure, and I am also very nervous about setting default routing parameters - in dev and production - just for the sake of this one feature. It seems too intrusive and risky, and I am not certain we won't break something in production. In other words, I don't understand it enough in order to intelligently assess the risks and possible side effects (such as breaking tests as we've seen).

So, I think we should restructure this to avoid using Rails url settings, and helper methods, outside of the view/controller environment.

One way would revert back to your original idea of using view helper(s) method to initiate convert the regular URL to a shortened URL. That might look like:

your view helper method is invoked in the view template
the helper method derives the standard (non-shortened) URL and calls a model method, passing in the URL and other parameters (ID's) as needed
The model tries to find the shortened URL in the DB and returns that if found
If not found, it invokes the shortening service, store the shortened URL and returns that

Your basically have the code described above already built - we just need to place a helper method in then mix in order to prevent the need to pull in URL helpers (and set config params) as the model level.

Of course, this was your first approach and I asked for a change that removed the helper method. So, my mistake. However, this is typical with how a lot of development proceeds - start to go down a path and, as you discover problems and risks, back up and adjust as needed.

tomurb added 2 commits July 10, 2018 14:04

tinyurl

93e3a86

forgot to add in last

1be6bc6

patmbolger reviewed Jul 10, 2018

View reviewed changes

tomurb added 2 commits July 13, 2018 12:46

shorten_url service, added columns

8facb79

Merge branch 'develop' into convert-images-links-to-shortened-urls#15…

2c5e3e5

…7840742 'updating'

houndci-bot reviewed Jul 13, 2018

View reviewed changes

tomurb closed this Jul 14, 2018

patmbolger reviewed Jul 14, 2018

View reviewed changes

tomurb added 2 commits July 19, 2018 12:31

Merge branch 'develop' into convert-images-links-to-shortened-urls#15…

eb3c901

…7840742

fixed ShortenUrl, company and user #short...url methods

d43264a

tomurb reopened this Jul 19, 2018

patmbolger requested changes Jul 20, 2018

View reviewed changes

fixes in models, extended error logs

abe568f

patmbolger reviewed Jul 21, 2018

View reviewed changes

tomurb added 3 commits July 21, 2018 12:49

ShortenUrl and spec fixes

0185302

cognitive complexity

12d8de9

Merge branch 'convert-images-links-to-shortened-urls#157840742' of ht…

f38808c

…tps://github.com/tmszrbn/shf-project into convert-images-links-to-shortened-urls#157840742

tomurb added 2 commits July 22, 2018 19:21

back to the helpers

d21f70e

just a little change

99b7c21

patmbolger approved these changes Jul 23, 2018

View reviewed changes

patmbolger merged commit 70b41fe into AgileVentures:develop Jul 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Convert image links to shortened URLs #546

[WIP] Convert image links to shortened URLs #546

tomurb commented Jul 9, 2018

tomurb commented Jul 10, 2018

patmbolger left a comment

patmbolger Jul 10, 2018

patmbolger Jul 10, 2018

patmbolger Jul 10, 2018

patmbolger left a comment

patmbolger Jul 10, 2018

patmbolger Jul 10, 2018

patmbolger Jul 10, 2018

patmbolger Jul 10, 2018

houndci-bot Jul 13, 2018

tomurb commented Jul 13, 2018 •

edited

tomurb commented Jul 14, 2018

patmbolger Jul 14, 2018

patmbolger Jul 14, 2018

tomurb Jul 14, 2018 •

edited

patmbolger Jul 15, 2018 •

edited

patmbolger Jul 19, 2018

patmbolger Jul 19, 2018

patmbolger Jul 19, 2018

patmbolger Jul 19, 2018

patmbolger Jul 20, 2018

patmbolger Jul 20, 2018

patmbolger Jul 20, 2018

patmbolger commented Jul 21, 2018

patmbolger Jul 21, 2018

patmbolger commented Jul 22, 2018

[WIP] Convert image links to shortened URLs #546

[WIP] Convert image links to shortened URLs #546

Conversation

tomurb commented Jul 9, 2018

tomurb commented Jul 10, 2018

patmbolger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patmbolger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomurb commented Jul 13, 2018 • edited

tomurb commented Jul 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomurb Jul 14, 2018 • edited

Choose a reason for hiding this comment

patmbolger Jul 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patmbolger commented Jul 21, 2018

Choose a reason for hiding this comment

patmbolger commented Jul 22, 2018

tomurb commented Jul 13, 2018 •

edited

tomurb Jul 14, 2018 •

edited

patmbolger Jul 15, 2018 •

edited