Skip to content

/otwarchive

AO3-4587 Error 404 when trying to edit or delete a bookmark for a deleted work #2476

Merged
merged 7 commits into from Jun 7, 2016

Labels

Priority: High Reviewed: Ready to Merge

Milestone

No milestone

Assignees

No one assigned

4 participants

@zz9pzza @shalott @sarken @houndci-bot
@zz9pzza
zz9pzza commented Jun 4, 2016

https://otwarchive.atlassian.net/browse/AO3-4587

zz9pzza added some commits Jun 4, 2016
@zz9pzza zz9pzza First write the failing test 607711b
@zz9pzza zz9pzza Proposed fix 71017a9
@zz9pzza zz9pzza Fix the delete path cd3f236
@zz9pzza zz9pzza Fix the delete test

Some checks were not successful

1 errored and 1 successful checks
Details continuous-integration/travis-ci/pr — The Travis CI build could not complete due to an error
hound — 5 violations found.
2b39ca8
@houndci-bot houndci-bot commented on the diff Jun 4, 2016
View full changes
app/models/bookmark.rb
@@ -90,8 +90,9 @@ class Bookmark < ActiveRecord::Base
after_save :invalidate_bookmark_count
def invalidate_bookmark_count
- if self.bookmarkable_type == 'Work'
- Work.find(self.bookmarkable_id).invalidate_public_bookmarks_count
+ work = Work.where(:id => self.bookmarkable_id)
@houndci-bot
houndci-bot added a note Jun 4, 2016

Use the new Ruby 1.9 hash syntax.
Redundant self detected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@houndci-bot houndci-bot commented on an outdated diff Jun 4, 2016
View full outdated diff
app/models/bookmark.rb
@@ -90,8 +90,9 @@ class Bookmark < ActiveRecord::Base
after_save :invalidate_bookmark_count
def invalidate_bookmark_count
- if self.bookmarkable_type == 'Work'
- Work.find(self.bookmarkable_id).invalidate_public_bookmarks_count
+ work = Work.where(:id => self.bookmarkable_id)
+ if work.present? && self.bookmarkable_type == 'Work'
@houndci-bot
houndci-bot added a note Jun 4, 2016

Unnecessary spacing detected.
Redundant self detected.
Operator && should be surrounded by a single space.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@zz9pzza zz9pzza Listen to the hound

All checks have passed

2 successful checks
Details continuous-integration/travis-ci/pr — The Travis CI build passed
hound — 3 violations found.
8906dd9
@shalott shalott and 1 other commented on an outdated diff Jun 6, 2016
View full outdated diff
app/views/bookmarks/confirm_delete.html.erb
@@ -3,7 +3,7 @@
<!--main content-->
<%= form_for(@bookmark, :html => {:method => :delete, :class => "simple destroy"}) do |f| %>
- <p class="caution notice"><%= ts('Are you sure you want to <strong><em>delete</em></strong> your bookmark of "%{bookmarkable}"?', :bookmarkable => @bookmark.bookmarkable.title).html_safe %></p>
+ <p class="caution notice"><%= ts('Are you sure you want to <strong><em>delete</em></strong> your bookmark of "%{bookmarkable}"?', :bookmarkable => (@bookmark.bookmarkable.nil? ? "This has been deleted, sorry!": @bookmark.bookmarkable.title)).html_safe %></p>
@shalott
Organization for Transformative Works member
shalott added a note Jun 6, 2016

This defeats the purpose of having it be in ts() though and also inserting "This has been deleted" into this sentence would look very odd. Doesn't this option only come up ON the bookmark itself? Do we actually need to include the name of the bookmarkable object? I suggest we remove it.

@sarken
Organization for Transformative Works member
sarken added a note Jun 6, 2016

This isn't on the bookmark itself, but rather a separate confirmation page for anyone who doesn't have JavaScript enabled, so it's relatively important to make sure users know what they're deleting (in case they have memory/cognitive issues, opened multiple delete windows, opened the window and walked away, etc). I'd suggest having two versions of the message, like:

<% if @bookmark.bookmarkable.nil? %>
  <%= ts('Are you sure you want to <strong><em>delete</em></strong> your bookmark of this deleted work?').html_safe %>
<% else %>
  <%= ts('Are you sure you want to <strong><em>delete</em></strong> your bookmark of "%{bookmarkable}"?', bookmarkable: @bookmark.bookmarkable.title).html_safe %>
<% end %>
@shalott
Organization for Transformative Works member
shalott added a note Jun 6, 2016

This looks good to me too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
zz9pzza added some commits Jun 6, 2016
@zz9pzza zz9pzza Merge branch 'master' of https://github.com/otwcode/otwarchive into A… 
…O3-4587
 05ac03e
@zz9pzza zz9pzza Listen to review

All checks have passed

2 successful checks
Details continuous-integration/travis-ci/pr — The Travis CI build passed
hound — 3 violations found.
c1638ee
@shalott
Organization for Transformative Works member
shalott commented Jun 7, 2016

Note this may also fix https://otwarchive.atlassian.net/browse/AO3-3788 and also fixes an issue where users who had a bookmark of a deleted work could not delete their accounts.

James, if you can fix the hash syntax that would be great but given the issues I am marking this ready to merge

@zz9pzza zz9pzza merged commit 0d1d2d5 into otwcode:master Jun 7, 2016
2 checks passed

2 checks passed

Details continuous-integration/travis-ci/pr The Travis CI build passed
hound 3 violations found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.