Prevent user seeing field length SQL error when creating a search track #2859

lizconlan · 2015-10-20T11:50:03Z

New migration to extend the length of the track_things.track_query field from 255 chars to 500

Validates that the query fits within the 500 char limit to avoid the user seeing a database error if they exceed the field length

Fixes #937

garethrees · 2015-10-20T12:03:41Z

spec/models/track_thing_spec.rb

@@ -70,6 +70,13 @@
    track_thing = TrackThing.create_track_for_search_query('fancy dog', 'bodies')
    expect(track_thing.track_query).to match(/variety:authority/)
  end
+
+  it "will check that the query isn't too long to store" do
+    long_query = "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Duis blandit consequat enim, sit amet facilisis lorem bibendum in. Pellentesque cursus pretium tristique. Mauris nec venenatis justo. Nam facilisis, lectus et blandit eleifend, felis arcu massa nunc. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Duis blandit consequat enim, sit amet facilisis lorem bibendum in. Pellentesque cursus pretium tristique. Mauris nec venenatis justo. Nam facilisis, lectus et blandit eleifend, felis arcu massa nunc."


Maybe just "Lorem " * 100?

oh yes, that's much less horrible - thanks!

houndci-bot · 2015-10-21T12:23:45Z

spec/controllers/track_controller_spec.rb

+  let(:ir) { FactoryGirl.create(:info_request,
+                                :title => 'My request',
+                                :url_title => 'myrequest') }
+  let(:track_thing) { FactoryGirl.create(:request_update_track,


Avoid using {...} for multi-line blocks.

houndci-bot · 2015-10-22T12:07:33Z

spec/controllers/track_controller_spec.rb

+end
+
+describe TrackController, "when making a search track" do
+  let(:track_thing) { FactoryGirl.create(:search_track,


Avoid using {...} for multi-line blocks.

crowbot · 2015-10-23T10:17:39Z

Nice work on cleaning up the specs. Now that the validation error is displayed in the UI, I can see that there are two things still to fix up here - errors.full_messages appends the attribute name to the error message making it a bit garbled. I think we want to define a custom message on the validation so that we can translate the error message with a _() call and present a complete clear string - so the track_controller code could be:

-flash[:error] = @track_thing.errors.full_messages.join(", ")
+flash[:error] = @track_thing.errors.map{ |attr,message| message }.join(", ")

It's always worth checking the results of a PR in a browser before submitting it for review.

lizconlan · 2015-10-23T16:34:37Z

bah, I was hoping (from a too-quick read of the docs) to get a humanized version of the message e.g. "Track query is too long" rather than just the message ("is too long") or the unexpected actual outcome "TrackThing|Track query is too long" which is pretty nasty :(

houndci-bot · 2015-10-27T11:22:38Z

spec/controllers/track_controller_spec.rb

+    it "should send localised alerts" do
+      # set the time the comment event happened at to within the last week
+      ire = info_request_events(:silly_comment_event)
+      ire.created_at = Time.now - 3.days


Do not use Time.now without zone. Use one of Time.zone.now, Time.now.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.

houndci-bot · 2015-10-27T11:22:39Z

spec/controllers/track_controller_spec.rb

-    response.headers['Expires'].should == '0'
-  end
+  describe "when making a search track" do
+    let(:track_thing) { FactoryGirl.create(:search_track,


Avoid using {...} for multi-line blocks.

I think there's an exemption for this rubocop/rubocop#376

I don't disagree with the rule as it happens – you can still do:

let(:blah) do # ... end

If you rebase on develop you should get the updated config that disables hound checks by default, other than the ones we've explicitly enabled though :)

houndci-bot · 2015-10-27T14:43:49Z

spec/controllers/track_controller_spec.rb

+    it "should send localised alerts" do
+      # set the time the comment event happened at to within the last week
+      ire = info_request_events(:silly_comment_event)
+      ire.created_at = Time.now - 3.days


Do not use Time.now without zone. Use one of Time.zone.now, Time.now.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.to_i, Time.now.to_f instead.

garethrees · 2015-10-27T17:58:36Z

Looks good to me other than the minor comment (#2859 (comment)) – 🚢

garethrees · 2015-10-28T10:55:32Z

Could also do with a changelog entry btw! :)

lizconlan changed the title ~~Prevent user from seeing field length SQL error when creating a search track~~ Prevent user seeing field length SQL error when creating a search track Oct 20, 2015

garethrees reviewed Oct 20, 2015
View reviewed changes

lizconlan added the awaiting-review label Oct 20, 2015

crowbot added 3 - current sprint and removed awaiting-review labels Oct 20, 2015

lizconlan added 4 - now and removed 3 - current sprint labels Oct 21, 2015

houndci-bot reviewed Oct 21, 2015
View reviewed changes

lizconlan force-pushed the 937-long-track-query-errors branch 3 times, most recently from e0c6805 to 83ed3c0 Compare October 22, 2015 12:07

houndci-bot reviewed Oct 22, 2015
View reviewed changes

lizconlan added awaiting-review and removed 3 - current sprint labels Oct 22, 2015

crowbot removed the awaiting-review label Oct 23, 2015

lizconlan force-pushed the 937-long-track-query-errors branch 3 times, most recently from 2dbaa1f to 4584689 Compare October 23, 2015 17:07

lizconlan added awaiting-review and removed awaiting-review labels Oct 26, 2015

houndci-bot reviewed Oct 27, 2015
View reviewed changes

lizconlan added the awaiting-review label Oct 27, 2015

houndci-bot reviewed Oct 27, 2015
View reviewed changes

lizconlan force-pushed the 937-long-track-query-errors branch from 610bb1e to e26a9f8 Compare October 27, 2015 14:47

lizconlan added 2 commits October 27, 2015 18:13

Increase length of track_query field

ce59268

Add max length validation for TrackThing#track_query

edf5403

lizconlan added awaiting-deploy and removed awaiting-review labels Oct 27, 2015

lizconlan force-pushed the 937-long-track-query-errors branch from df63f8d to c5be6ad Compare October 27, 2015 19:02

lizconlan added 2 commits October 28, 2015 10:23

TrackController#track_set returns errors in flash when saving

63bcc6f

Tidy up TrackController spec

928b7ed

lizconlan force-pushed the 937-long-track-query-errors branch from c5be6ad to 928b7ed Compare October 28, 2015 10:48

Update changelog

a1f814a

lizconlan merged commit a1f814a into develop Oct 28, 2015

lizconlan removed the awaiting-deploy label Oct 28, 2015

lizconlan deleted the 937-long-track-query-errors branch October 28, 2015 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent user seeing field length SQL error when creating a search track #2859

Prevent user seeing field length SQL error when creating a search track #2859

lizconlan commented Oct 20, 2015

garethrees Oct 20, 2015

lizconlan Oct 20, 2015

houndci-bot Oct 21, 2015

houndci-bot Oct 22, 2015

crowbot commented Oct 23, 2015

lizconlan commented Oct 23, 2015

houndci-bot Oct 27, 2015

houndci-bot Oct 27, 2015

lizconlan Oct 27, 2015

garethrees Oct 27, 2015

houndci-bot Oct 27, 2015

garethrees commented Oct 27, 2015

garethrees commented Oct 28, 2015

Prevent user seeing field length SQL error when creating a search track #2859

Prevent user seeing field length SQL error when creating a search track #2859

Conversation

lizconlan commented Oct 20, 2015

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

crowbot commented Oct 23, 2015

lizconlan commented Oct 23, 2015

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

garethrees commented Oct 27, 2015

garethrees commented Oct 28, 2015