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

notes: fix notes breaking with long title #1496

Merged
merged 5 commits into from
Sep 12, 2019

Conversation

diegoviola
Copy link
Contributor

Fixes: #1491

@benwbrum
Copy link
Owner

benwbrum commented Sep 5, 2019

This works great, but we need to catch an HTTP 302 error as well as a 500.

To reproduce the 302:

  • Log in to the site in browser tab A
  • Add a note to a page (should succeed)
  • Open a new browser tab B on the site using the same browser/same session
  • Log out of the site
  • On browser tab A, try to add a new note
  • The server catches that the session is logged out and returns a 302, but nothing is displayed to the user.

Copy link
Owner

@benwbrum benwbrum left a comment

Choose a reason for hiding this comment

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

The truncation and error handling works great, but I noticed a new need to show errors to users who have been logged out.

@diegoviola
Copy link
Contributor Author

diegoviola commented Sep 5, 2019

* The server catches that the session is logged out and returns a `302`, but nothing is displayed to the user.

I followed the exact steps that you provided but I can't reproduce the 302 error.

Instead, I get a 200 OK and a TypeError: xhr.responseJSON is undefined.

@benwbrum
Copy link
Owner

benwbrum commented Sep 6, 2019

I'm definitely seeing a 302.

Here's another way to reproduce this:

  • Log in
  • Navigate to the Transcribe tab of a page
  • Log out
  • Click the browser back button to go back to the (stale) Transcribe tab
  • Add a note and save
  • See the spinner

Here's what I see in the logs when I do this:

Started POST "/notes" for 127.0.0.1 at 2019-09-05 19:30:54 -0500
Processing by NotesController#create as JS
  Parameters: {"utf8"=>"✓", "page_id"=>"6086", "note"=>{"body"=>"this is a test"}, "button"=>""}
  Page Load (0.9ms)  SELECT  `pages`.* FROM `pages`  WHERE `pages`.`id` = 6086 LIMIT 1
  Work Load (0.7ms)  SELECT  `works`.* FROM `works`  WHERE `works`.`id` = 17 LIMIT 1
  Collection Load (0.5ms)  SELECT  `collections`.* FROM `collections`  WHERE `collections`.`id` = 1 LIMIT 1
  IaWork Load (0.4ms)  SELECT  `ia_works`.* FROM `ia_works`  WHERE `ia_works`.`work_id` = 17 LIMIT 1
http://www.archive.org/services/find_file.php?file=1939fieldnotesla00klau
DEBUG Server=ia800304.us.archive.org
DEBUG Dir=/0/items/1939fieldnotesla00klau
DEBUG: ia_server = {"server"=>"ia800304.us.archive.org", "ia_path"=>"/0/items/1939fieldnotesla00klau"}
  OmekaItem Load (0.6ms)  SELECT  `omeka_items`.* FROM `omeka_items`  WHERE `omeka_items`.`work_id` = 17 LIMIT 1
  PageBlock Load (0.5ms)  SELECT `page_blocks`.* FROM `page_blocks`  WHERE `page_blocks`.`controller` = 'notes' AND `page_blocks`.`view` = 'create'
Redirected to http://localhost:3000/bpoc/field-notes-of-laurence-m-klauber/1939-field-notes-laurence-m-klauber/display/6086
  Visit Load (0.5ms)  SELECT  `visits`.* FROM `visits`  WHERE `visits`.`visit_token` = '1cff0df4-cbfc-4e62-8090-09e384bff5d3'  ORDER BY `visits`.`id` ASC LIMIT 1
   (0.4ms)  BEGIN
  SQL (0.9ms)  INSERT INTO `ahoy_events` (`name`, `properties`, `time`, `visit_id`) VALUES ('notes#create', '{\"collection_id\":1,\"collection_title\":\"Field Notes of Laurence M. Klauber\",\"work_id\":17,\"work_title\":\"1939 Field Notes Laurence M. Klauber\",\"page_id\":6086,\"page_title\":\"Tuesday, February 7, 1939\"}', '2019-09-06 00:30:55', 18)
   (4.7ms)  COMMIT
Completed 302 Found in 589ms (ActiveRecord: 10.1ms)
Oink Action: notes#create
Memory usage: 875448 | PID: 14203
Instantiation Breakdown: Total: 7 | Page: 1 | Work: 1 | Collection: 1 | IaWork: 1 | Note: 1 | Visit: 1 | Ahoy::Event: 1
Oink Log Entry Complete

@diegoviola diegoviola force-pushed the 1491-fix-notes-breaking-with-long-title branch from 2b79185 to 5f5418a Compare September 6, 2019 17:35
@diegoviola
Copy link
Contributor Author

Just for the record, I think the 302 is produced from the redirect:

Filter chain halted as :authorized? rendered or redirected
Completed 302 Found in 22ms (ActiveRecord: 8.4ms)

I can't see that from the xhr, the ajax response only sees a "200 OK" with the login page in the xhr response, but at that point it's probably too late to catch the redirect.

@benwbrum please let me know if the latest commit fixes the issue, it seems to be working for me.

@diegoviola diegoviola force-pushed the 1491-fix-notes-breaking-with-long-title branch from 3ecbef5 to a2aa518 Compare September 7, 2019 04:29
@benwbrum benwbrum assigned benwbrum and unassigned diegoviola Sep 10, 2019
@benwbrum benwbrum merged commit f74c73d into development Sep 12, 2019
@benwbrum benwbrum deleted the 1491-fix-notes-breaking-with-long-title branch September 12, 2019 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants