From 0a48c8d2a83da65b87c5d2c1e7f17e27afc3ab2d Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 12:56:20 -0300
Subject: [PATCH 01/27] Remove other_user existence condition from
 bookmark.visible_to? function

---
 app/models/bookmark.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb
index 0709e9577f..5de5ce9d4a 100644
--- a/app/models/bookmark.rb
+++ b/app/models/bookmark.rb
@@ -14,10 +14,9 @@ class Bookmark < ApplicationRecord
   scope :visible_to, ->(user) { _visible_user(user)._visible_post(user) }
 
   def visible_to?(other_user)
-    return false unless other_user
     return false unless post.visible_to?(other_user)
     return false unless reply
-    return true if other_user.id == user.id
+    return true if other_user.try(:id) == user.id
     user.public_bookmarks
   end
 end

From 2f5fdc052a3be4dc582ed7880c8d6af1f6be73f6 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 12:59:06 -0300
Subject: [PATCH 02/27] Use visible_to? function in replies GET bookmark

---
 app/controllers/api/v1/replies_controller.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/controllers/api/v1/replies_controller.rb b/app/controllers/api/v1/replies_controller.rb
index c5558331a9..f83bc361cd 100644
--- a/app/controllers/api/v1/replies_controller.rb
+++ b/app/controllers/api/v1/replies_controller.rb
@@ -27,16 +27,16 @@ def index
   api :GET, '/replies/:id/bookmark', "Load a user's bookmark attached to a reply if it exists and is visible"
   param :id, :number, required: true, desc: "Reply ID"
   param :user_id, :number, required: true, desc: "User ID"
-  error 403, "Reply's post or user's bookmarks are not visible"
+  error 403, "Reply's post is not visible to the user"
   error 404, "Reply or user not found"
   error 422, "Invalid parameters provided"
   def bookmark
     return unless (reply = find_object(Reply))
     return unless (user = find_object(User, param: :user_id))
     access_denied and return unless reply.post.visible_to?(current_user)
-    bookmark_not_found and return unless user.public_bookmarks || user.id == current_user.try(:id)
 
     bookmark = reply.bookmarks.find_by(user_id: user.id, type: "reply_bookmark")
+    bookmark_not_found and return unless bookmark.visible_to?(current_user)
 
     if bookmark.present?
       render json: bookmark.as_json

From e08636a85909e8446ca98f9be53fbe5a0442a24a Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 13:02:13 -0300
Subject: [PATCH 03/27] Remove unnecessary checks in users GET bookmarks

---
 app/controllers/api/v1/users_controller.rb | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb
index f6b0a7c423..6d5806afa7 100644
--- a/app/controllers/api/v1/users_controller.rb
+++ b/app/controllers/api/v1/users_controller.rb
@@ -41,12 +41,9 @@ def posts
   param :id, :number, required: true, desc: "User ID"
   param :post_id, :number, required: false, desc: "Post ID"
   param :page, :number, required: false, desc: 'Page in results (25 per page)'
-  error 403, "Bookmarks are not visible to the user"
   error 404, "User not found"
   error 422, "Invalid parameters provided"
   def bookmarks
-    render json: { bookmarks: [] } and return unless @user.public_bookmarks || @user.id == current_user.try(:id)
-
     bookmarks = @user.bookmarks.visible_to(current_user)
     bookmarks = bookmarks.where(post_id: params[:post_id]) if params[:post_id].present?
 

From c44c500f41f326dc62c5d277bc0beed1538a2963 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 13:49:56 -0300
Subject: [PATCH 04/27] Create scope to find per-reply bookmark visibility

---
 app/controllers/bookmarks_controller.rb | 8 ++++----
 app/models/reply.rb                     | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb
index 0c323c16c6..161c47626a 100644
--- a/app/controllers/bookmarks_controller.rb
+++ b/app/controllers/bookmarks_controller.rb
@@ -11,20 +11,20 @@ def search
     use_javascript('bookmarks/rename')
     return unless params[:commit].present?
     return unless (@user = User.find_by_id(params[:user_id]))
-    unless @user.id == current_user.try(:id) || @user.public_bookmarks
+
+    @search_results = @user.bookmarked_replies.bookmark_visible_to(@user, current_user)
+    if @search_results.empty?
       # Return empty list when a user's bookmarks are private
-      @search_results = Reply.none.paginate(page: 1)
+      @search_results = @search_results.paginate(page: 1)
       return
     end
 
-    @search_results = @user.bookmarked_replies
     if params[:post_id].present?
       @posts = Post.where(id: params[:post_id])
       @search_results = @search_results.where(post_id: params[:post_id])
     end
 
     @search_results = @search_results
-      .visible_to(current_user)
       .joins(:post)
       .order('posts.subject, replies.created_at, posts.id')
       .joins(:user)
diff --git a/app/models/reply.rb b/app/models/reply.rb
index c1eb7c7bb8..99880e16fb 100644
--- a/app/models/reply.rb
+++ b/app/models/reply.rb
@@ -30,6 +30,7 @@ class Reply < ApplicationRecord
   )
 
   scope :visible_to, ->(user) { where(post_id: Post.visible_to(user).select(:id)) }
+  scope :bookmark_visible_to, ->(bookmark_owner, viewing_user) { where(bookmarks: bookmark_owner.bookmarks.visible_to(viewing_user)) }
 
   def post_page(per=25)
     per_page = per > 0 ? per : post.replies.count

From 334d091a166ef29ce6621cc42c5db7de5e4ce450 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 14:04:16 -0300
Subject: [PATCH 05/27] Check for bookmark existence in replies GET bookmark

---
 app/controllers/api/v1/replies_controller.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/controllers/api/v1/replies_controller.rb b/app/controllers/api/v1/replies_controller.rb
index f83bc361cd..73cc6cea3c 100644
--- a/app/controllers/api/v1/replies_controller.rb
+++ b/app/controllers/api/v1/replies_controller.rb
@@ -36,7 +36,7 @@ def bookmark
     access_denied and return unless reply.post.visible_to?(current_user)
 
     bookmark = reply.bookmarks.find_by(user_id: user.id, type: "reply_bookmark")
-    bookmark_not_found and return unless bookmark.visible_to?(current_user)
+    bookmark_not_found and return unless bookmark&.visible_to?(current_user)
 
     if bookmark.present?
       render json: bookmark.as_json

From 66c8e246d03fbf6d14e1dceaf2fa29af0dae4877 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 14:07:07 -0300
Subject: [PATCH 06/27] Use individual bookmark publicness to determine
 visibility

---
 app/models/bookmark.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb
index 5de5ce9d4a..4047a13d0a 100644
--- a/app/models/bookmark.rb
+++ b/app/models/bookmark.rb
@@ -11,9 +11,10 @@ class Bookmark < ApplicationRecord
 
   scope :_visible_user, ->(user) { where(user_id: user&.id).joins(:user).or(where(user: { public_bookmarks: true })) }
   scope :_visible_post, ->(user) { where(post_id: Post.visible_to(user).select(:id)) }
-  scope :visible_to, ->(user) { _visible_user(user)._visible_post(user) }
+  scope :visible_to, ->(user) { _visible_user(user)._visible_post(user).or(where(public: true)) }
 
   def visible_to?(other_user)
+    return true if public
     return false unless post.visible_to?(other_user)
     return false unless reply
     return true if other_user.try(:id) == user.id

From c2e349f21f6eb52fffc026faf10f1276a7a5c403 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 14:37:01 -0300
Subject: [PATCH 07/27] Update public_bookmarks label in user#edit

---
 app/views/users/edit.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/users/edit.haml b/app/views/users/edit.haml
index fd8a0d8879..cefa3b22fc 100644
--- a/app/views/users/edit.haml
+++ b/app/views/users/edit.haml
@@ -88,7 +88,7 @@
         %th.sub Bookmarks
         %td{class: cycle('even', 'odd')}
           = f.check_box :public_bookmarks, class: 'width-15 vmid'
-          = f.label :public_bookmarks, "Make bookmarks public", class: 'vmid'
+          = f.label :public_bookmarks, "Make all bookmarks public", class: 'vmid'
       %tr
         %th.sub Character Quick Switcher
         %td{class: cycle('even', 'odd')}

From 8efdf2af5d49725d90cda1bdc741d56143459466 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 15:03:43 -0300
Subject: [PATCH 08/27] Add UI to edit whether a bookmark is individually
 public

---
 app/assets/javascripts/bookmarks/rename.js    | 24 ++++++++++++-------
 .../api/v1/bookmarks_controller.rb            |  7 +++---
 app/controllers/bookmarks_controller.rb       |  2 +-
 app/views/bookmarks/search.haml               | 15 ++++++++----
 spec/system/bookmarks/search_spec.rb          | 22 ++++++++---------
 5 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/app/assets/javascripts/bookmarks/rename.js b/app/assets/javascripts/bookmarks/rename.js
index 933a072091..e16808a1f1 100644
--- a/app/assets/javascripts/bookmarks/rename.js
+++ b/app/assets/javascripts/bookmarks/rename.js
@@ -1,17 +1,22 @@
 const originalValues = {};
+const originalPublicCheckboxes = {};
 
 $(document).ready(function() {
   const textFields = $(`.bookmark-name-text-field`);
   for (const textField of textFields) {
     originalValues[textField.dataset.bookmarkId] = $(textField).val();
   }
+  const checkBoxes = $(`.bookmark-public-checkbox`);
+  for (const checkBox of checkBoxes) {
+    originalPublicCheckboxes[checkBox.dataset.bookmarkId] = $(checkBox).prop("checked");
+  }
 
-  if (!$(".rename-bookmark").length) return;
+  if (!$(".edit-bookmark").length) return;
 
-  $(".rename-bookmark").click(function() {
+  $(".edit-bookmark").click(function() {
     /* Button to rename a bookmark */
     const bookmarkId = this.dataset.bookmarkId;
-    const editors = $(`.bookmark-name-editor`);
+    const editors = $(`.bookmark-editor`);
     for (const editor of editors) {
       // Hide all editors other than the one clicked
       const editorId = editor.dataset.bookmarkId;
@@ -25,25 +30,27 @@ $(document).ready(function() {
 
     // Toggle the one clicked
     $(`.bookmark-name[data-bookmark-id="${bookmarkId}"]`).toggle();
-    $(`.bookmark-name-editor[data-bookmark-id="${bookmarkId}"]`).toggle();
+    $(`.bookmark-editor[data-bookmark-id="${bookmarkId}"]`).toggle();
     return false;
   });
 
-  $(".save-bookmark-name").click(function() {
+  $(".save-bookmark").click(function() {
     $(".loading").show();
 
     const bookmarkId = this.dataset.bookmarkId;
     const newName = $(`.bookmark-name-text-field[data-bookmark-id="${bookmarkId}"]`).val();
+    const newPublic = $(`.bookmark-public-checkbox[data-bookmark-id="${bookmarkId}"]`).prop("checked");
 
     $.authenticatedAjax({
       url: '/api/v1/bookmarks/'+bookmarkId,
       type: 'PATCH',
-      data: {'name': newName},
+      data: {'name': newName, 'public': newPublic},
       success: function(data) {
         originalValues[bookmarkId] = newName;
+        originalPublicCheckboxes[bookmarkId] = newPublic;
         $(".loading").hide();
         $(`.bookmark-name[data-bookmark-id="${bookmarkId}"] span`).first().html(nameFromData(data.name));
-        $(`.bookmark-name-editor[data-bookmark-id="${bookmarkId}"]`).hide();
+        $(`.bookmark-editor[data-bookmark-id="${bookmarkId}"]`).hide();
         $(`.bookmark-name[data-bookmark-id="${bookmarkId}"]`).show();
         $(`.saveconf[data-bookmark-id="${bookmarkId}"]`).show().delay(2000).fadeOut();
       },
@@ -56,10 +63,11 @@ $(document).ready(function() {
     return false;
   });
 
-  $(".discard-bookmark-name").click(function() {
+  $(".discard-bookmark-changes").click(function() {
     const bookmarkId = this.dataset.bookmarkId;
     if (confirm('Are you sure you wish to discard your changes?')) {
       $(`.bookmark-name-text-field[data-bookmark-id="${bookmarkId}"]`).val(originalValues[bookmarkId]);
+      $(`.bookmark-public-checkbox[data-bookmark-id="${bookmarkId}"]`).prop("checked", originalPublicCheckboxes[bookmarkId]);
     }
     return false;
   });
diff --git a/app/controllers/api/v1/bookmarks_controller.rb b/app/controllers/api/v1/bookmarks_controller.rb
index 7abbed1dd7..2c4a51044a 100644
--- a/app/controllers/api/v1/bookmarks_controller.rb
+++ b/app/controllers/api/v1/bookmarks_controller.rb
@@ -32,10 +32,11 @@ def create
     render json: bookmark.as_json
   end
 
-  api :PATCH, '/bookmarks/:id', 'Update a single bookmark. Currently only supports renaming.'
+  api :PATCH, '/bookmarks/:id', 'Update a single bookmark.'
   header 'Authorization', 'Authorization token for a user in the format "Authorization" : "Bearer [token]"', required: true
   param :id, :number, required: true, desc: "Bookmark ID"
-  param :name, String, required: true, allow_blank: true, desc: "Bookmark's new name"
+  param :name, String, required: false, allow_blank: true, desc: "Bookmark's new name"
+  param :public, :boolean, required: false, allow_blank: true, desc: "Bookmark's new public status"
   error 403, "Bookmark is not visible to the user"
   error 404, "Bookmark not found"
   error 422, "Invalid parameters provided"
@@ -45,7 +46,7 @@ def update
       return
     end
 
-    unless @bookmark.update(name: params[:name])
+    unless @bookmark.update(params.permit(:name, :public))
       error = { message: 'Bookmark could not be updated.' }
       render json: { errors: [error] }, status: :unprocessable_entity
       return
diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb
index 161c47626a..76bd36c3b6 100644
--- a/app/controllers/bookmarks_controller.rb
+++ b/app/controllers/bookmarks_controller.rb
@@ -29,7 +29,7 @@ def search
       .order('posts.subject, replies.created_at, posts.id')
       .joins(:user)
       .left_outer_joins(:character)
-      .select('replies.*, bookmarks.name as bookmark_name, bookmarks.id as bookmark_id, characters.name, ' \
+      .select('replies.*, bookmarks.id as bookmark_id, bookmarks.name as bookmark_name, bookmarks.public as bookmark_public, characters.name, ' \
               'characters.screenname, users.username, users.deleted as user_deleted')
       .paginate(page: page)
 
diff --git a/app/views/bookmarks/search.haml b/app/views/bookmarks/search.haml
index be82429541..84b73ddabd 100644
--- a/app/views/bookmarks/search.haml
+++ b/app/views/bookmarks/search.haml
@@ -53,10 +53,15 @@
                         = image_tag "icons/accept.png", title: 'Saved', class: 'vmid', alt: ''
                         Saved
                     - if user_is_owner
-                      .bookmark-name-editor.hidden{ data: { bookmark_id: reply.bookmark_id } }
-                        = text_field_tag nil, reply.bookmark_name, class: "bookmark-name-text-field", data: { bookmark_id: reply.bookmark_id }
-                        = submit_tag 'Save', class: 'save-bookmark-name button', data: { bookmark_id: reply.bookmark_id }
-                        = submit_tag 'Discard Changes', class: 'discard-bookmark-name button', data: { bookmark_id: reply.bookmark_id }
+                      .bookmark-editor.hidden{ data: { bookmark_id: reply.bookmark_id } }
+                        = text_field_tag nil, reply.bookmark_name, placeholder: "Bookmark Name", class: "bookmark-name-text-field vmid", data: { bookmark_id: reply.bookmark_id }
+                        &nbsp;&nbsp;&nbsp;
+                        = check_box_tag :public, true, reply.bookmark_public, class: "bookmark-public-checkbox vmid", data: { bookmark_id: reply.bookmark_id }
+                        %span.vmid
+                          Public
+                        &nbsp;&nbsp;&nbsp;&nbsp;
+                        = submit_tag 'Save', class: 'save-bookmark button vmid', data: { bookmark_id: reply.bookmark_id }
+                        = submit_tag 'Discard Changes', class: 'discard-bookmark-changes button vmid', data: { bookmark_id: reply.bookmark_id }
                         .loading.float-right.margin-top-7.hidden
                           = image_tag 'icons/loading.gif', title: 'Loading...', class: 'vmid', alt: 'Loading...'
                         .saveerror.float-right.margin-top-7.hidden{ data: { bookmark_id: reply.bookmark_id } }
@@ -79,7 +84,7 @@
                       = link_to reply_path(reply, anchor: "reply-#{reply.id}") do
                         = image_tag "icons/link.png".freeze, title: 'Permalink'.freeze, alt: 'Permalink'.freeze
                       - if user_is_owner
-                        = image_tag "icons/bookmark_pencil.png".freeze, title: 'Rename Bookmark', alt: 'Rename Bookmark', class: 'rename-bookmark pointer', data: { bookmark_id: reply.bookmark_id }
+                        = image_tag "icons/bookmark_pencil.png".freeze, title: 'Rename Bookmark', alt: 'Rename Bookmark', class: 'edit-bookmark pointer', data: { bookmark_id: reply.bookmark_id }
                       - if (bookmark = reply.bookmark_by(current_user))
                         = link_to bookmark_path(bookmark), method: :delete do
                           = image_tag "icons/bookmark_delete.png".freeze, title: 'Remove Bookmark'.freeze, alt: 'Remove Bookmark'.freeze
diff --git a/spec/system/bookmarks/search_spec.rb b/spec/system/bookmarks/search_spec.rb
index a65bce4b96..9ba93c384d 100644
--- a/spec/system/bookmarks/search_spec.rb
+++ b/spec/system/bookmarks/search_spec.rb
@@ -151,27 +151,27 @@ def validate_bookmarks_found(bookmarks, posts, condensed: false)
     # Can toggle bookmark name editor
     first_bookmark = private_bookmarks.first
     first_bookmark_name_text_field = find(".bookmark-name-text-field[data-bookmark-id='#{first_bookmark.id}']", visible: false)
-    first_bookmark_edit_name_button = find(".rename-bookmark[data-bookmark-id='#{first_bookmark.id}']")
+    first_bookmark_edit_button = find(".edit-bookmark[data-bookmark-id='#{first_bookmark.id}']")
     expect(first_bookmark_name_text_field).not_to be_visible
-    first_bookmark_edit_name_button.click
+    first_bookmark_edit_button.click
     expect(first_bookmark_name_text_field).to be_visible
-    first_bookmark_edit_name_button.click
+    first_bookmark_edit_button.click
     expect(first_bookmark_name_text_field).not_to be_visible
 
     # Toggling one bookmark untoggles another
     last_bookmark = private_bookmarks.last
     last_bookmark_name_text_field = find(".bookmark-name-text-field[data-bookmark-id='#{last_bookmark.id}']", visible: false)
-    last_bookmark_edit_name_button = find(".rename-bookmark[data-bookmark-id='#{last_bookmark.id}']")
+    last_bookmark_edit_button = find(".edit-bookmark[data-bookmark-id='#{last_bookmark.id}']")
     expect(last_bookmark_name_text_field).not_to be_visible
-    last_bookmark_edit_name_button.click
+    last_bookmark_edit_button.click
     expect(last_bookmark_name_text_field).to be_visible
-    first_bookmark_edit_name_button.click
+    first_bookmark_edit_button.click
     expect(first_bookmark_name_text_field).to be_visible
     expect(last_bookmark_name_text_field).not_to be_visible
-    first_bookmark_edit_name_button.click
+    first_bookmark_edit_button.click
 
     # Can rename bookmark
-    first_bookmark_edit_name_button.click
+    first_bookmark_edit_button.click
     new_bookmark_name = "New Bookmark Name #{first_bookmark.id}"
     first_bookmark_name_text_field.set(new_bookmark_name)
     click_button "Save"
@@ -180,14 +180,14 @@ def validate_bookmarks_found(bookmarks, posts, condensed: false)
     expect(find(".bookmark-name[data-bookmark-id='#{first_bookmark.id}']")).to have_text(new_bookmark_name)
 
     # Discarding changes to the name works
-    first_bookmark_edit_name_button.click
+    first_bookmark_edit_button.click
     first_bookmark_name_text_field.set(new_bookmark_name + "different")
     accept_alert { click_button "Discard Changes" }
     expect(first_bookmark_name_text_field.value).to eq(new_bookmark_name)
-    first_bookmark_edit_name_button.click
+    first_bookmark_edit_button.click
 
     # Can clear bookmark name
-    first_bookmark_edit_name_button.click
+    first_bookmark_edit_button.click
     first_bookmark_name_text_field.set("")
     click_button "Save"
     expect(find(".bookmark-name[data-bookmark-id='#{first_bookmark.id}']")).to have_text("(Unnamed)")

From bf0fc7eed59cce2d0839713f675e277945d2d52a Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 15:21:32 -0300
Subject: [PATCH 09/27] Simplify create logic

---
 app/controllers/bookmarks_controller.rb | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb
index 76bd36c3b6..5f357c28e4 100644
--- a/app/controllers/bookmarks_controller.rb
+++ b/app/controllers/bookmarks_controller.rb
@@ -53,11 +53,7 @@ def create
     bookmark = Bookmark.where(reply_id: @reply.id, user_id: current_user.id, post_id: @reply.post.id,
       type: 'reply_bookmark',).first_or_initialize
     if bookmark.new_record?
-      if params[:bookmark_name].present?
-        bookmark.update!(name: params[:bookmark_name])
-      else
-        bookmark.save!
-      end
+      bookmark.update!(params.permit(:name))
       flash[:success] = "Bookmark added."
     else
       flash[:error] = "Bookmark already exists."

From 9cff3653f0e8f3adf71389cf7036fb603cf3852a Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 15:22:04 -0300
Subject: [PATCH 10/27] Allow :public param to be passed to POST create

---
 app/controllers/bookmarks_controller.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb
index 5f357c28e4..4f7cac1d7e 100644
--- a/app/controllers/bookmarks_controller.rb
+++ b/app/controllers/bookmarks_controller.rb
@@ -53,7 +53,7 @@ def create
     bookmark = Bookmark.where(reply_id: @reply.id, user_id: current_user.id, post_id: @reply.post.id,
       type: 'reply_bookmark',).first_or_initialize
     if bookmark.new_record?
-      bookmark.update!(params.permit(:name))
+      bookmark.update!(params.permit(:name, :public))
       flash[:success] = "Bookmark added."
     else
       flash[:error] = "Bookmark already exists."

From 19a68c993ab4fec3fefee698565b596a3f2b359c Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 15:38:27 -0300
Subject: [PATCH 11/27] Properly copy bookmark name

---
 app/views/bookmarks/search.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/bookmarks/search.haml b/app/views/bookmarks/search.haml
index 84b73ddabd..dd59922a41 100644
--- a/app/views/bookmarks/search.haml
+++ b/app/views/bookmarks/search.haml
@@ -89,7 +89,7 @@
                         = link_to bookmark_path(bookmark), method: :delete do
                           = image_tag "icons/bookmark_delete.png".freeze, title: 'Remove Bookmark'.freeze, alt: 'Remove Bookmark'.freeze
                       - else
-                        = link_to bookmarks_path(at_id: reply.id, bookmark_name: reply.bookmark_name), method: :post do
+                        = link_to bookmarks_path(at_id: reply.id, name: reply.bookmark_name), method: :post do
                           = image_tag "icons/bookmark.png".freeze, title: 'Copy Bookmark'.freeze, alt: 'Copy Bookmark'.freeze
                     .post-content
                       = sanitize_written_content(reply.content, reply.editor_mode)

From 2e27ad9d3bb3b99109384175baa9a36e6b8cc827 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 15:40:16 -0300
Subject: [PATCH 12/27] Add/update tests

---
 .../api/v1/bookmarks_controller_spec.rb       | 49 ++++++++++++++-----
 .../api/v1/replies_controller_spec.rb         | 21 ++++++--
 .../api/v1/users_controller_spec.rb           | 23 ++++++---
 spec/controllers/bookmarks/create_spec.rb     | 13 ++++-
 spec/system/bookmarks/search_spec.rb          | 27 +++++++++-
 5 files changed, 108 insertions(+), 25 deletions(-)

diff --git a/spec/controllers/api/v1/bookmarks_controller_spec.rb b/spec/controllers/api/v1/bookmarks_controller_spec.rb
index a30494ab1f..4429477ca3 100644
--- a/spec/controllers/api/v1/bookmarks_controller_spec.rb
+++ b/spec/controllers/api/v1/bookmarks_controller_spec.rb
@@ -114,16 +114,6 @@
       expect(response.parsed_body['errors'][0]['message']).to eq("You do not have permission to perform this action.")
     end
 
-    it "requires name param", :show_in_doc do
-      api_login
-      reply = create(:reply)
-      bookmark = create(:bookmark, reply: reply, post: reply.post)
-      bookmark.user.update!(public_bookmarks: true)
-      patch :update, params: { id: bookmark.id }
-      expect(response).to have_http_status(422)
-      expect(response.parsed_body['errors'][0]['message']).to eq("Missing parameter name")
-    end
-
     it "requires ownership of bookmark", :show_in_doc do
       api_login
       reply = create(:reply)
@@ -150,18 +140,21 @@
       expect(response.parsed_body['errors'][0]['message']).to eq('Bookmark could not be updated.')
     end
 
-    it "succeeds with valid bookmark", :show_in_doc do
+    it "succeeds with only name", :show_in_doc do
       user = api_login
       reply = create(:reply)
       bookmark = create(:bookmark, reply: reply, post: reply.post, user: user)
       expect(bookmark.name).to be_nil
+      expect(bookmark.public).to be false
 
       patch :update, params: { id: bookmark.id, name: "New name" }
 
       expect(response).to have_http_status(200)
       expect(response.parsed_body['name']).to eq("New name")
+      expect(response.parsed_body['public']).to be false
       bookmark.reload
       expect(bookmark.name).to eq('New name')
+      expect(bookmark.public).to be false
     end
 
     it "accepts blank name", :show_in_doc do
@@ -176,6 +169,40 @@
       bookmark.reload
       expect(bookmark.name).to eq('')
     end
+
+    it "succeeds with only public", :show_in_doc do
+      user = api_login
+      reply = create(:reply)
+      bookmark = create(:bookmark, reply: reply, post: reply.post, user: user, name: "Old name")
+      expect(bookmark.name).to eq("Old name")
+      expect(bookmark.public).to be false
+
+      patch :update, params: { id: bookmark.id, public: true }
+
+      expect(response).to have_http_status(200)
+      expect(response.parsed_body['name']).to eq("Old name")
+      expect(response.parsed_body['public']).to be true
+      bookmark.reload
+      expect(bookmark.name).to eq('Old name')
+      expect(bookmark.public).to be true
+    end
+
+    it "succeeds with both parameters", :show_in_doc do
+      user = api_login
+      reply = create(:reply)
+      bookmark = create(:bookmark, reply: reply, post: reply.post, user: user, name: "Old name")
+      expect(bookmark.name).to eq("Old name")
+      expect(bookmark.public).to be false
+
+      patch :update, params: { id: bookmark.id, name: "New name", public: true }
+
+      expect(response).to have_http_status(200)
+      expect(response.parsed_body['name']).to eq("New name")
+      expect(response.parsed_body['public']).to be true
+      bookmark.reload
+      expect(bookmark.name).to eq('New name')
+      expect(bookmark.public).to be true
+    end
   end
 
   describe "DELETE destroy" do
diff --git a/spec/controllers/api/v1/replies_controller_spec.rb b/spec/controllers/api/v1/replies_controller_spec.rb
index 77ba12ae93..c44e8a58a9 100644
--- a/spec/controllers/api/v1/replies_controller_spec.rb
+++ b/spec/controllers/api/v1/replies_controller_spec.rb
@@ -71,21 +71,32 @@
       expect(response.parsed_body['errors'][0]['message']).to eq("You do not have permission to perform this action.")
     end
 
-    it "fails with private bookmarks", :show_in_doc do
-      get :bookmark, params: { id: create(:reply).id, user_id: create(:user).id }
+    it "fails if bookmark does not exist", :show_in_doc do
+      get :bookmark, params: { id: create(:reply).id, user_id: create(:user, public_bookmarks: true).id }
       expect(response).to have_http_status(404)
       expect(response.parsed_body['errors'].size).to eq(1)
       expect(response.parsed_body['errors'][0]['message']).to eq("Bookmark could not be found.")
     end
 
-    it "fails if bookmark does not exist", :show_in_doc do
-      get :bookmark, params: { id: create(:reply).id, user_id: create(:user, public_bookmarks: true).id }
+    it "fails if bookmark is private", :show_in_doc do
+      bookmark = create(:bookmark, user: create(:user))
+      get :bookmark, params: { id: bookmark.reply.id, user_id: bookmark.user.id }
       expect(response).to have_http_status(404)
       expect(response.parsed_body['errors'].size).to eq(1)
       expect(response.parsed_body['errors'][0]['message']).to eq("Bookmark could not be found.")
     end
 
-    it "succeeds with public bookmarks", :show_in_doc do
+    it "succeeds if bookmark is public", :show_in_doc do
+      bookmark = create(:bookmark, user: create(:user), public: true)
+      get :bookmark, params: { id: bookmark.reply.id, user_id: bookmark.user.id }
+      expect(response).to have_http_status(200)
+      expect(response.parsed_body['id']).to eq(bookmark.id)
+      expect(response.parsed_body['user_id']).to eq(bookmark.user.id)
+      expect(response.parsed_body['reply_id']).to eq(bookmark.reply_id)
+      expect(response.parsed_body['post_id']).to eq(bookmark.post_id)
+    end
+
+    it "succeeds if user bookmarks are public", :show_in_doc do
       bookmark = create(:bookmark, user: create(:user, public_bookmarks: true))
       get :bookmark, params: { id: bookmark.reply.id, user_id: bookmark.user.id }
       expect(response).to have_http_status(200)
diff --git a/spec/controllers/api/v1/users_controller_spec.rb b/spec/controllers/api/v1/users_controller_spec.rb
index 2169525ff5..d4973baffa 100644
--- a/spec/controllers/api/v1/users_controller_spec.rb
+++ b/spec/controllers/api/v1/users_controller_spec.rb
@@ -142,24 +142,35 @@ def create_search_users
       expect(response.parsed_body['bookmarks'].size).to eq(0)
     end
 
-    it "succeeds with public bookmarks", :show_in_doc do
+    it "succeeds if bookmark is public", :show_in_doc do
+      user = create(:user)
+      create(:bookmark, user: user, public: true)
+      get :bookmarks, params: { id: user.id }
+      expect(response).to have_http_status(200)
+      expect(response.parsed_body['bookmarks'].size).to eq(1)
+    end
+
+    it "succeeds if user bookmarks are public", :show_in_doc do
       user = create(:user, public_bookmarks: true)
+      create(:bookmark, user: user)
       get :bookmarks, params: { id: user.id }
       expect(response).to have_http_status(200)
-      expect(response.parsed_body['bookmarks'].size).to eq(0)
+      expect(response.parsed_body['bookmarks'].size).to eq(1)
     end
 
     it "succeeds with own bookmarks", :show_in_doc do
       user = api_login
+      create(:bookmark, user: user)
       get :bookmarks, params: { id: user.id }
       expect(response).to have_http_status(200)
-      expect(response.parsed_body['bookmarks'].size).to eq(0)
+      expect(response.parsed_body['bookmarks'].size).to eq(1)
     end
 
-    it "filters non-visible bookmarks", :show_in_doc do
-      user = create(:user, public_bookmarks: true)
-      bookmarks = create_list(:bookmark, 2, user: user)
+    it "filters non-public bookmarks", :show_in_doc do
+      user = create(:user)
+      bookmarks = create_list(:bookmark, 3, user: user)
       bookmarks[0].post.update!(privacy: :private)
+      bookmarks[1].update!(public: true)
       get :bookmarks, params: { id: user.id }
       expect(response).to have_http_status(200)
       expect(response.parsed_body['bookmarks'].size).to eq(1)
diff --git a/spec/controllers/bookmarks/create_spec.rb b/spec/controllers/bookmarks/create_spec.rb
index cd7396c083..84661d6d21 100644
--- a/spec/controllers/bookmarks/create_spec.rb
+++ b/spec/controllers/bookmarks/create_spec.rb
@@ -37,11 +37,22 @@
 
   it "succeeds with name" do
     login
-    post :create, params: { at_id: reply.id, bookmark_name: "new bookmark" }
+    post :create, params: { at_id: reply.id, name: "new bookmark" }
     expect(response).to redirect_to(reply_url(reply, anchor: "reply-#{reply.id}"))
     expect(flash[:success]).to eq('Bookmark added.')
     bookmark = Bookmark.order(:id).last
     expect(bookmark.name).to eq("new bookmark")
+    expect(bookmark.public).to be false
+  end
+
+  it "succeeds with public" do
+    login
+    post :create, params: { at_id: reply.id, public: true }
+    expect(response).to redirect_to(reply_url(reply, anchor: "reply-#{reply.id}"))
+    expect(flash[:success]).to eq('Bookmark added.')
+    bookmark = Bookmark.order(:id).last
+    expect(bookmark.name).to be_nil
+    expect(bookmark.public).to be true
   end
 
   it "fails if already exists" do
diff --git a/spec/system/bookmarks/search_spec.rb b/spec/system/bookmarks/search_spec.rb
index 9ba93c384d..e50446c359 100644
--- a/spec/system/bookmarks/search_spec.rb
+++ b/spec/system/bookmarks/search_spec.rb
@@ -118,6 +118,14 @@ def validate_bookmarks_found(bookmarks, posts, condensed: false)
     expect(page).to have_text("0 results")
     within(".paginator") { expect(page).to have_text("Total: 0") }
 
+    # Searching for a private user's public bookmarks shows results
+    private_bookmarks.first.update!(public: true)
+    private_bookmarks.last.update!(public: true)
+    perform_search user: private_user
+    validate_bookmarks_found [private_bookmarks.first, private_bookmarks.last], [private_bookmarks.first.post, private_bookmarks.last.post]
+    private_bookmarks.first.update!(public: false)
+    private_bookmarks.last.update!(public: false)
+
     # Searching for a public user's bookmarks does show results
     perform_search user: public_user
     validate_bookmarks_found public_bookmarks, posts
@@ -177,13 +185,28 @@ def validate_bookmarks_found(bookmarks, posts, condensed: false)
     click_button "Save"
     expect(find(".saveconf[data-bookmark-id='#{first_bookmark.id}']")).to be_visible
     expect(first_bookmark_name_text_field).not_to be_visible
+    refresh
     expect(find(".bookmark-name[data-bookmark-id='#{first_bookmark.id}']")).to have_text(new_bookmark_name)
 
-    # Discarding changes to the name works
+    # Can toggle bookmark publicness
+    first_bookmark_edit_button.click
+    first_bookmark_public_checkbox = find(".bookmark-public-checkbox[data-bookmark-id='#{first_bookmark.id}']")
+    first_bookmark_public_checkbox.click
+    click_button "Save"
+    expect(find(".saveconf[data-bookmark-id='#{first_bookmark.id}']")).to be_visible
+    refresh
+    first_bookmark_edit_button.click
+    expect(first_bookmark_public_checkbox).to be_checked
+    first_bookmark_public_checkbox.click
+    click_button "Save"
+
+    # Discarding changes works
     first_bookmark_edit_button.click
     first_bookmark_name_text_field.set(new_bookmark_name + "different")
+    first_bookmark_public_checkbox.click
     accept_alert { click_button "Discard Changes" }
     expect(first_bookmark_name_text_field.value).to eq(new_bookmark_name)
+    expect(first_bookmark_public_checkbox).not_to be_checked
     first_bookmark_edit_button.click
 
     # Can clear bookmark name
@@ -201,7 +224,7 @@ def validate_bookmarks_found(bookmarks, posts, condensed: false)
     # Can copy another user's bookmark
     perform_search user: public_user
     new_bookmark = public_bookmarks.detect { |bookmark| bookmark.name.present? && !bookmark.reply.bookmark_by(private_user) }
-    click_link(href: bookmarks_path(at_id: new_bookmark.reply.id, bookmark_name: new_bookmark.name.presence))
+    click_link(href: bookmarks_path(at_id: new_bookmark.reply.id, name: new_bookmark.name.presence))
     perform_search user: private_user
     expect(page).to have_link(new_bookmark.reply.user.username, href: user_path(new_bookmark.reply.user))
     expect(page).to have_link(href: reply_path(new_bookmark.reply, anchor: "reply-#{new_bookmark.reply.id}"), count: 1)

From 688a2490f35244a3593d1d10942eca4655831226 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 15:53:10 -0300
Subject: [PATCH 13/27] Add label to public checkbox

---
 app/views/bookmarks/search.haml | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/app/views/bookmarks/search.haml b/app/views/bookmarks/search.haml
index dd59922a41..ad6705ff0c 100644
--- a/app/views/bookmarks/search.haml
+++ b/app/views/bookmarks/search.haml
@@ -56,9 +56,8 @@
                       .bookmark-editor.hidden{ data: { bookmark_id: reply.bookmark_id } }
                         = text_field_tag nil, reply.bookmark_name, placeholder: "Bookmark Name", class: "bookmark-name-text-field vmid", data: { bookmark_id: reply.bookmark_id }
                         &nbsp;&nbsp;&nbsp;
-                        = check_box_tag :public, true, reply.bookmark_public, class: "bookmark-public-checkbox vmid", data: { bookmark_id: reply.bookmark_id }
-                        %span.vmid
-                          Public
+                        = check_box_tag "public_#{reply.bookmark_id}", true, reply.bookmark_public, class: "bookmark-public-checkbox vmid", data: { bookmark_id: reply.bookmark_id }
+                        = label_tag "public_#{reply.bookmark_id}", 'Public', class: "vmid"
                         &nbsp;&nbsp;&nbsp;&nbsp;
                         = submit_tag 'Save', class: 'save-bookmark button vmid', data: { bookmark_id: reply.bookmark_id }
                         = submit_tag 'Discard Changes', class: 'discard-bookmark-changes button vmid', data: { bookmark_id: reply.bookmark_id }

From 07e898cb08ca74f104a122284c39acad3f542953 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 16:06:10 -0300
Subject: [PATCH 14/27] Fix display of bookmark edit form fields and buttons

---
 app/assets/stylesheets/replies.scss |  6 ++++++
 app/views/bookmarks/search.haml     | 25 +++++++++++++------------
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/app/assets/stylesheets/replies.scss b/app/assets/stylesheets/replies.scss
index e10c82cfde..830fd80685 100644
--- a/app/assets/stylesheets/replies.scss
+++ b/app/assets/stylesheets/replies.scss
@@ -562,3 +562,9 @@ div.post-subheader { width: 100%; }
   width: 100%;
   height: 50px;
 }
+
+/* Bookmark Edit Form */
+.bookmark-editor-fields { margin-right: 20px; }
+@media (max-width: 850px) {
+  .bookmark-editor-fields, .bookmark-editor-buttons { display: block; margin-right: 0; }
+}
diff --git a/app/views/bookmarks/search.haml b/app/views/bookmarks/search.haml
index ad6705ff0c..7971479655 100644
--- a/app/views/bookmarks/search.haml
+++ b/app/views/bookmarks/search.haml
@@ -54,18 +54,19 @@
                         Saved
                     - if user_is_owner
                       .bookmark-editor.hidden{ data: { bookmark_id: reply.bookmark_id } }
-                        = text_field_tag nil, reply.bookmark_name, placeholder: "Bookmark Name", class: "bookmark-name-text-field vmid", data: { bookmark_id: reply.bookmark_id }
-                        &nbsp;&nbsp;&nbsp;
-                        = check_box_tag "public_#{reply.bookmark_id}", true, reply.bookmark_public, class: "bookmark-public-checkbox vmid", data: { bookmark_id: reply.bookmark_id }
-                        = label_tag "public_#{reply.bookmark_id}", 'Public', class: "vmid"
-                        &nbsp;&nbsp;&nbsp;&nbsp;
-                        = submit_tag 'Save', class: 'save-bookmark button vmid', data: { bookmark_id: reply.bookmark_id }
-                        = submit_tag 'Discard Changes', class: 'discard-bookmark-changes button vmid', data: { bookmark_id: reply.bookmark_id }
-                        .loading.float-right.margin-top-7.hidden
-                          = image_tag 'icons/loading.gif', title: 'Loading...', class: 'vmid', alt: 'Loading...'
-                        .saveerror.float-right.margin-top-7.hidden{ data: { bookmark_id: reply.bookmark_id } }
-                          = image_tag "icons/exclamation.png", title: 'Error', class: 'vmid', alt: ''
-                          Error, please try again
+                        %span.bookmark-editor-fields
+                          = text_field_tag nil, reply.bookmark_name, placeholder: "Bookmark Name", class: "bookmark-name-text-field vmid", data: { bookmark_id: reply.bookmark_id }
+                          &nbsp;&nbsp;&nbsp;
+                          = check_box_tag "public_#{reply.bookmark_id}", true, reply.bookmark_public, class: "bookmark-public-checkbox vmid", data: { bookmark_id: reply.bookmark_id }
+                          = label_tag "public_#{reply.bookmark_id}", 'Public', class: "vmid"
+                        %span.bookmark-editor-buttons
+                          = submit_tag 'Save', class: 'save-bookmark button vmid', data: { bookmark_id: reply.bookmark_id }
+                          = submit_tag 'Discard Changes', class: 'discard-bookmark-changes button vmid', data: { bookmark_id: reply.bookmark_id }
+                          .loading.float-right.margin-top-7.hidden
+                            = image_tag 'icons/loading.gif', title: 'Loading...', class: 'vmid', alt: 'Loading...'
+                          .saveerror.float-right.margin-top-7.hidden{ data: { bookmark_id: reply.bookmark_id } }
+                            = image_tag "icons/exclamation.png", title: 'Error', class: 'vmid', alt: ''
+                            Error, please try again
                 %tr
                   %td.vtop{ class: cycle('even'.freeze, 'odd'.freeze) }
                     .post-info-box

From 2559577111a172adf0983d6e6d2f96e7554eef86 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 18:27:51 -0300
Subject: [PATCH 15/27] Remove unnecessary conditional from replies GET
 bookmark

---
 app/controllers/api/v1/replies_controller.rb | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/app/controllers/api/v1/replies_controller.rb b/app/controllers/api/v1/replies_controller.rb
index 73cc6cea3c..826816fe4a 100644
--- a/app/controllers/api/v1/replies_controller.rb
+++ b/app/controllers/api/v1/replies_controller.rb
@@ -38,11 +38,7 @@ def bookmark
     bookmark = reply.bookmarks.find_by(user_id: user.id, type: "reply_bookmark")
     bookmark_not_found and return unless bookmark&.visible_to?(current_user)
 
-    if bookmark.present?
-      render json: bookmark.as_json
-    else
-      bookmark_not_found
-    end
+    render json: bookmark.as_json
   end
 
   private

From cba330baec0012821839b3999bb6bc577a1e6167 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 18:46:40 -0300
Subject: [PATCH 16/27] Rename variable for clarity

---
 app/assets/javascripts/bookmarks/rename.js | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/assets/javascripts/bookmarks/rename.js b/app/assets/javascripts/bookmarks/rename.js
index e16808a1f1..caa1a5937e 100644
--- a/app/assets/javascripts/bookmarks/rename.js
+++ b/app/assets/javascripts/bookmarks/rename.js
@@ -1,10 +1,10 @@
-const originalValues = {};
+const originalNameTextboxes = {};
 const originalPublicCheckboxes = {};
 
 $(document).ready(function() {
   const textFields = $(`.bookmark-name-text-field`);
   for (const textField of textFields) {
-    originalValues[textField.dataset.bookmarkId] = $(textField).val();
+    originalNameTextboxes[textField.dataset.bookmarkId] = $(textField).val();
   }
   const checkBoxes = $(`.bookmark-public-checkbox`);
   for (const checkBox of checkBoxes) {
@@ -46,7 +46,7 @@ $(document).ready(function() {
       type: 'PATCH',
       data: {'name': newName, 'public': newPublic},
       success: function(data) {
-        originalValues[bookmarkId] = newName;
+        originalNameTextboxes[bookmarkId] = newName;
         originalPublicCheckboxes[bookmarkId] = newPublic;
         $(".loading").hide();
         $(`.bookmark-name[data-bookmark-id="${bookmarkId}"] span`).first().html(nameFromData(data.name));
@@ -66,7 +66,7 @@ $(document).ready(function() {
   $(".discard-bookmark-changes").click(function() {
     const bookmarkId = this.dataset.bookmarkId;
     if (confirm('Are you sure you wish to discard your changes?')) {
-      $(`.bookmark-name-text-field[data-bookmark-id="${bookmarkId}"]`).val(originalValues[bookmarkId]);
+      $(`.bookmark-name-text-field[data-bookmark-id="${bookmarkId}"]`).val(originalNameTextboxes[bookmarkId]);
       $(`.bookmark-public-checkbox[data-bookmark-id="${bookmarkId}"]`).prop("checked", originalPublicCheckboxes[bookmarkId]);
     }
     return false;

From fc2a5b1325549fc8f2d83a0f1a2bd942a60155e7 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 18:49:36 -0300
Subject: [PATCH 17/27] Change order of validations of visible_to? so as not to
 override post visibility

---
 app/models/bookmark.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb
index 4047a13d0a..e194a4055b 100644
--- a/app/models/bookmark.rb
+++ b/app/models/bookmark.rb
@@ -14,9 +14,9 @@ class Bookmark < ApplicationRecord
   scope :visible_to, ->(user) { _visible_user(user)._visible_post(user).or(where(public: true)) }
 
   def visible_to?(other_user)
-    return true if public
     return false unless post.visible_to?(other_user)
     return false unless reply
+    return true if public
     return true if other_user.try(:id) == user.id
     user.public_bookmarks
   end

From 214f7a968c8195309f69aa6ffdbe012b72d881c6 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 19:01:01 -0300
Subject: [PATCH 18/27] Fix order of validation of bookmark :visible_to scope

---
 app/models/bookmark.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb
index e194a4055b..3a9e6d87e0 100644
--- a/app/models/bookmark.rb
+++ b/app/models/bookmark.rb
@@ -11,7 +11,7 @@ class Bookmark < ApplicationRecord
 
   scope :_visible_user, ->(user) { where(user_id: user&.id).joins(:user).or(where(user: { public_bookmarks: true })) }
   scope :_visible_post, ->(user) { where(post_id: Post.visible_to(user).select(:id)) }
-  scope :visible_to, ->(user) { _visible_user(user)._visible_post(user).or(where(public: true)) }
+  scope :visible_to, ->(user) { _visible_user(user).or(where(public: true))._visible_post(user) }
 
   def visible_to?(other_user)
     return false unless post.visible_to?(other_user)

From 13b1a84286151fa56d1ded14271d82198a63d749 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 19:07:46 -0300
Subject: [PATCH 19/27] Add test for conflicts between private post and public
 bookmark

---
 spec/controllers/api/v1/bookmarks_controller_spec.rb | 10 ++++++++++
 spec/controllers/api/v1/replies_controller_spec.rb   |  9 +++++++++
 spec/controllers/api/v1/users_controller_spec.rb     | 10 ++++++++++
 3 files changed, 29 insertions(+)

diff --git a/spec/controllers/api/v1/bookmarks_controller_spec.rb b/spec/controllers/api/v1/bookmarks_controller_spec.rb
index 4429477ca3..6528bda6eb 100644
--- a/spec/controllers/api/v1/bookmarks_controller_spec.rb
+++ b/spec/controllers/api/v1/bookmarks_controller_spec.rb
@@ -140,6 +140,16 @@
       expect(response.parsed_body['errors'][0]['message']).to eq('Bookmark could not be updated.')
     end
 
+    it "Public bookmark doesn't override private post", :show_in_doc do
+      user = api_login
+      bookmark = create(:bookmark, user: user, public: true)
+      bookmark.post.update!(privacy: :private)
+
+      patch :update, params: { id: bookmark.id, name: "New name" }
+      expect(response).to have_http_status(403)
+      expect(response.parsed_body['errors'][0]['message']).to eq("You do not have permission to perform this action.")
+    end
+
     it "succeeds with only name", :show_in_doc do
       user = api_login
       reply = create(:reply)
diff --git a/spec/controllers/api/v1/replies_controller_spec.rb b/spec/controllers/api/v1/replies_controller_spec.rb
index c44e8a58a9..39763054e2 100644
--- a/spec/controllers/api/v1/replies_controller_spec.rb
+++ b/spec/controllers/api/v1/replies_controller_spec.rb
@@ -96,6 +96,15 @@
       expect(response.parsed_body['post_id']).to eq(bookmark.post_id)
     end
 
+    it "Public bookmark doesn't override private post", :show_in_doc do
+      bookmark = create(:bookmark, user: create(:user), public: true)
+      bookmark.post.update!(privacy: :private)
+      get :bookmark, params: { id: bookmark.reply.id, user_id: bookmark.user.id }
+      expect(response).to have_http_status(403)
+      expect(response.parsed_body['errors'].size).to eq(1)
+      expect(response.parsed_body['errors'][0]['message']).to eq("You do not have permission to perform this action.")
+    end
+
     it "succeeds if user bookmarks are public", :show_in_doc do
       bookmark = create(:bookmark, user: create(:user, public_bookmarks: true))
       get :bookmark, params: { id: bookmark.reply.id, user_id: bookmark.user.id }
diff --git a/spec/controllers/api/v1/users_controller_spec.rb b/spec/controllers/api/v1/users_controller_spec.rb
index d4973baffa..3082bbeaef 100644
--- a/spec/controllers/api/v1/users_controller_spec.rb
+++ b/spec/controllers/api/v1/users_controller_spec.rb
@@ -177,6 +177,16 @@ def create_search_users
       expect(response.parsed_body['bookmarks'][0]['id']).to eq(bookmarks[1].id)
     end
 
+    it "Public bookmark doesn't override private post", :show_in_doc do
+      user = create(:user)
+      bookmarks = create_list(:bookmark, 2, user: user, public: true)
+      bookmarks[0].post.update!(privacy: :private)
+      get :bookmarks, params: { id: user.id }
+      expect(response).to have_http_status(200)
+      expect(response.parsed_body['bookmarks'].size).to eq(1)
+      expect(response.parsed_body['bookmarks'][0]['id']).to eq(bookmarks[1].id)
+    end
+
     it "filters post", :show_in_doc do
       user = create(:user, public_bookmarks: true)
       bookmarks = create_list(:bookmark, 3, user: user)

From 83bf8ca7e2bc963632e421294eb47372f5860e4e Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 19:15:30 -0300
Subject: [PATCH 20/27] Remove unnecessary arguments to create(:bookmark) spec
 calls

---
 .../api/v1/bookmarks_controller_spec.rb       | 36 +++++++------------
 spec/controllers/bookmarks/create_spec.rb     |  4 +--
 spec/controllers/bookmarks/destroy_spec.rb    |  2 +-
 spec/system/bookmarks/search_spec.rb          | 18 +++++-----
 spec/system/posts/show_spec.rb                |  2 +-
 5 files changed, 26 insertions(+), 36 deletions(-)

diff --git a/spec/controllers/api/v1/bookmarks_controller_spec.rb b/spec/controllers/api/v1/bookmarks_controller_spec.rb
index 6528bda6eb..5e58afcf11 100644
--- a/spec/controllers/api/v1/bookmarks_controller_spec.rb
+++ b/spec/controllers/api/v1/bookmarks_controller_spec.rb
@@ -59,12 +59,11 @@
 
     it "updates existing bookmark", :show_in_doc do
       user = api_login
-      reply = create(:reply)
 
-      bookmark = create(:bookmark, reply: reply, post: reply.post, user: user)
+      bookmark = create(:bookmark, user: user)
       expect(bookmark.name).to be_nil
 
-      post :create, params: { reply_id: reply.id, name: "New Name" }
+      post :create, params: { reply_id: bookmark.reply.id, name: "New Name" }
       expect(response).to have_http_status(200)
 
       expect(Bookmark.find_by_id(bookmark.id).name).to eq("New Name")
@@ -106,9 +105,8 @@
 
     it "requires visible bookmark", :show_in_doc do
       api_login
-      reply = create(:reply)
-      bookmark = create(:bookmark, reply: reply, post: reply.post)
-      reply.post.update!(privacy: :private)
+      bookmark = create(:bookmark)
+      bookmark.post.update!(privacy: :private)
       patch :update, params: { id: bookmark.id }
       expect(response).to have_http_status(403)
       expect(response.parsed_body['errors'][0]['message']).to eq("You do not have permission to perform this action.")
@@ -116,8 +114,7 @@
 
     it "requires ownership of bookmark", :show_in_doc do
       api_login
-      reply = create(:reply)
-      bookmark = create(:bookmark, reply: reply, post: reply.post)
+      bookmark = create(:bookmark)
       bookmark.user.update!(public_bookmarks: true)
       patch :update, params: { id: bookmark.id, name: "New" }
       expect(response).to have_http_status(403)
@@ -126,8 +123,7 @@
 
     it "handles failed saves" do
       user = api_login
-      reply = create(:reply)
-      bookmark = create(:bookmark, reply: reply, post: reply.post, user: user)
+      bookmark = create(:bookmark, user: user)
 
       allow(Bookmark).to receive(:find_by).and_call_original
       allow(Bookmark).to receive(:find_by).with({ id: bookmark.id.to_s }).and_return(bookmark)
@@ -152,8 +148,7 @@
 
     it "succeeds with only name", :show_in_doc do
       user = api_login
-      reply = create(:reply)
-      bookmark = create(:bookmark, reply: reply, post: reply.post, user: user)
+      bookmark = create(:bookmark, user: user)
       expect(bookmark.name).to be_nil
       expect(bookmark.public).to be false
 
@@ -169,8 +164,7 @@
 
     it "accepts blank name", :show_in_doc do
       user = api_login
-      reply = create(:reply)
-      bookmark = create(:bookmark, reply: reply, post: reply.post, user: user, name: "Old name")
+      bookmark = create(:bookmark, user: user, name: "Old name")
 
       patch :update, params: { id: bookmark.id, name: "" }
 
@@ -182,8 +176,7 @@
 
     it "succeeds with only public", :show_in_doc do
       user = api_login
-      reply = create(:reply)
-      bookmark = create(:bookmark, reply: reply, post: reply.post, user: user, name: "Old name")
+      bookmark = create(:bookmark, user: user, name: "Old name")
       expect(bookmark.name).to eq("Old name")
       expect(bookmark.public).to be false
 
@@ -199,8 +192,7 @@
 
     it "succeeds with both parameters", :show_in_doc do
       user = api_login
-      reply = create(:reply)
-      bookmark = create(:bookmark, reply: reply, post: reply.post, user: user, name: "Old name")
+      bookmark = create(:bookmark, user: user, name: "Old name")
       expect(bookmark.name).to eq("Old name")
       expect(bookmark.public).to be false
 
@@ -232,9 +224,8 @@
 
     it "requires visible bookmark", :show_in_doc do
       api_login
-      reply = create(:reply)
-      bookmark = create(:bookmark, reply: reply, post: reply.post)
-      reply.post.update!(privacy: :private)
+      bookmark = create(:bookmark)
+      bookmark.post.update!(privacy: :private)
       delete :destroy, params: { id: bookmark.id }
       expect(response).to have_http_status(403)
       expect(response.parsed_body['errors'][0]['message']).to eq("You do not have permission to perform this action.")
@@ -242,8 +233,7 @@
 
     it "requires ownership of bookmark", :show_in_doc do
       api_login
-      reply = create(:reply)
-      bookmark = create(:bookmark, user: create(:user, public_bookmarks: true), reply: reply, post: reply.post)
+      bookmark = create(:bookmark, user: create(:user, public_bookmarks: true))
       delete :destroy, params: { id: bookmark.id }
       expect(response).to have_http_status(403)
       expect(response.parsed_body['errors'][0]['message']).to eq("You do not have permission to perform this action.")
diff --git a/spec/controllers/bookmarks/create_spec.rb b/spec/controllers/bookmarks/create_spec.rb
index 84661d6d21..d686f24c05 100644
--- a/spec/controllers/bookmarks/create_spec.rb
+++ b/spec/controllers/bookmarks/create_spec.rb
@@ -57,7 +57,7 @@
 
   it "fails if already exists" do
     login_as(user)
-    existing_bookmark = create(:bookmark, user: user, reply: reply, post: reply.post, type: "reply_bookmark")
+    existing_bookmark = create(:bookmark, user: user, reply: reply)
     post :create, params: { at_id: reply.id }
     expect(response).to redirect_to(reply_url(reply, anchor: "reply-#{reply.id}"))
     expect(flash[:error]).to eq("Bookmark already exists.")
@@ -66,7 +66,7 @@
   end
 
   it "allows multiple users to bookmark the same reply" do
-    existing_bookmark = create(:bookmark, user: user, reply: reply, post: reply.post, type: "reply_bookmark")
+    existing_bookmark = create(:bookmark, user: user, reply: reply)
     login
     post :create, params: { at_id: reply.id }
     expect(response).to redirect_to(reply_url(reply, anchor: "reply-#{reply.id}"))
diff --git a/spec/controllers/bookmarks/destroy_spec.rb b/spec/controllers/bookmarks/destroy_spec.rb
index 6ed5c5ac51..1cd8cb79d9 100644
--- a/spec/controllers/bookmarks/destroy_spec.rb
+++ b/spec/controllers/bookmarks/destroy_spec.rb
@@ -1,7 +1,7 @@
 RSpec.describe BookmarksController, 'DELETE destroy' do
   let(:user) { create(:user, public_bookmarks: true) }
   let(:reply) { create(:reply) }
-  let(:bookmark) { create(:bookmark, user: user, reply: reply, post: reply.post) }
+  let(:bookmark) { create(:bookmark, user: user, reply: reply) }
 
   it "requires login" do
     delete :destroy, params: { id: -1 }
diff --git a/spec/system/bookmarks/search_spec.rb b/spec/system/bookmarks/search_spec.rb
index e50446c359..82030bed95 100644
--- a/spec/system/bookmarks/search_spec.rb
+++ b/spec/system/bookmarks/search_spec.rb
@@ -14,19 +14,19 @@
   end
   let!(:private_bookmarks) do
     [
-      create(:bookmark, user: private_user, reply: replies[1], post: replies[1].post),
-      create(:bookmark, user: private_user, reply: replies[3], post: replies[3].post),
-      create(:bookmark, user: private_user, reply: replies[4], post: replies[4].post, name: "Bookmark Name 1"),
-      create(:bookmark, user: private_user, reply: replies[6], post: replies[6].post, name: "Bookmark Name 2"),
+      create(:bookmark, user: private_user, reply: replies[1]),
+      create(:bookmark, user: private_user, reply: replies[3]),
+      create(:bookmark, user: private_user, reply: replies[4], name: "Bookmark Name 1"),
+      create(:bookmark, user: private_user, reply: replies[6], name: "Bookmark Name 2"),
     ]
   end
   let!(:public_bookmarks) do
     [
-      create(:bookmark, user: public_user, reply: replies[2], post: replies[2].post, name: "Bookmark Name 3"),
-      create(:bookmark, user: public_user, reply: replies[4], post: replies[5].post),
-      create(:bookmark, user: public_user, reply: replies[5], post: replies[5].post),
-      create(:bookmark, user: public_user, reply: replies[7], post: replies[7].post),
-      create(:bookmark, user: public_user, reply: replies[8], post: replies[8].post),
+      create(:bookmark, user: public_user, reply: replies[2], name: "Bookmark Name 3"),
+      create(:bookmark, user: public_user, reply: replies[4]),
+      create(:bookmark, user: public_user, reply: replies[5]),
+      create(:bookmark, user: public_user, reply: replies[7]),
+      create(:bookmark, user: public_user, reply: replies[8]),
     ]
   end
 
diff --git a/spec/system/posts/show_spec.rb b/spec/system/posts/show_spec.rb
index 26967945c7..39e41bce85 100644
--- a/spec/system/posts/show_spec.rb
+++ b/spec/system/posts/show_spec.rb
@@ -79,7 +79,7 @@
     let!(:user) { create(:user, password: 'known') }
     let!(:post) { create(:post) }
     let!(:reply) { create(:reply, post: post) }
-    let!(:bookmark) { create(:bookmark, reply: reply, post: post, user: user) }
+    let!(:bookmark) { create(:bookmark, reply: reply, user: user) }
 
     scenario "when logged out" do
       visit post_path(post)

From 41c70844a8074a9face678860f94bcdfb514c69a Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 19:26:27 -0300
Subject: [PATCH 21/27] Allow API endpoint for creating a new bookmark to set
 its publicness directly

---
 .../api/v1/bookmarks_controller.rb            |  6 ++--
 .../api/v1/bookmarks_controller_spec.rb       | 31 +++++++++++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/app/controllers/api/v1/bookmarks_controller.rb b/app/controllers/api/v1/bookmarks_controller.rb
index 2c4a51044a..02647bd3fd 100644
--- a/app/controllers/api/v1/bookmarks_controller.rb
+++ b/app/controllers/api/v1/bookmarks_controller.rb
@@ -7,10 +7,11 @@ class Api::V1::BookmarksController < Api::ApiController
     description 'Viewing and modifying bookmarks'
   end
 
-  api :POST, '/bookmarks', 'Create a bookmark for the current user at a reply. If one already exists, update its name.'
+  api :POST, '/bookmarks', 'Create a bookmark for the current user at a reply. If one already exists, update it.'
   header 'Authorization', 'Authorization token for a user in the format "Authorization" : "Bearer [token]"', required: true
   param :reply_id, :number, required: true, desc: "Reply ID"
   param :name, String, required: false, allow_blank: true, desc: "New bookmark's name"
+  param :public, :boolean, required: false, allow_blank: true, desc: "New bookmark's public status"
   error 403, "Reply is not visible to the user"
   error 404, "Reply not found"
   error 422, "Invalid parameters provided"
@@ -22,8 +23,7 @@ def create
     end
 
     bookmark = Bookmark.where(user: current_user, reply: reply, post: reply.post, type: "reply_bookmark").first_or_initialize
-    bookmark.assign_attributes(name: params[:name])
-    unless bookmark.save
+    unless bookmark.update(params.permit(:name, :public))
       error = { message: 'Bookmark could not be created.' }
       render json: { errors: [error] }, status: :unprocessable_entity
       return
diff --git a/spec/controllers/api/v1/bookmarks_controller_spec.rb b/spec/controllers/api/v1/bookmarks_controller_spec.rb
index 5e58afcf11..47ff2c3f32 100644
--- a/spec/controllers/api/v1/bookmarks_controller_spec.rb
+++ b/spec/controllers/api/v1/bookmarks_controller_spec.rb
@@ -43,6 +43,7 @@
       expect(bookmark.user).to eq(user)
       expect(bookmark.type).to eq("reply_bookmark")
       expect(bookmark.name).to be_nil
+      expect(bookmark.public).to be false
     end
 
     it "succeeds with name param", :show_in_doc do
@@ -54,7 +55,37 @@
 
       bookmark = Bookmark.find_by_id(response.parsed_body['id'])
       expect(bookmark.reply).to eq(reply)
+      expect(bookmark.post).to eq(reply.post)
       expect(bookmark.name).to eq("New Bookmark")
+      expect(bookmark.public).to be false
+    end
+
+    it "succeeds with public param", :show_in_doc do
+      api_login
+      reply = create(:reply)
+
+      post :create, params: { reply_id: reply.id, public: true }
+      expect(response).to have_http_status(200)
+
+      bookmark = Bookmark.find_by_id(response.parsed_body['id'])
+      expect(bookmark.reply).to eq(reply)
+      expect(bookmark.post).to eq(reply.post)
+      expect(bookmark.name).to be_nil
+      expect(bookmark.public).to be true
+    end
+
+    it "succeeds with both params", :show_in_doc do
+      api_login
+      reply = create(:reply)
+
+      post :create, params: { reply_id: reply.id, name: "New Bookmark", public: true }
+      expect(response).to have_http_status(200)
+
+      bookmark = Bookmark.find_by_id(response.parsed_body['id'])
+      expect(bookmark.reply).to eq(reply)
+      expect(bookmark.post).to eq(reply.post)
+      expect(bookmark.name).to eq("New Bookmark")
+      expect(bookmark.public).to be true
     end
 
     it "updates existing bookmark", :show_in_doc do

From 3eaf2d53424b6affe92b01c71f35b9b179fbe140 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 19:27:43 -0300
Subject: [PATCH 22/27] Remove unnecessary lookup of post ID when upserting
 bookmark through POST create

---
 app/controllers/api/v1/bookmarks_controller.rb | 5 +++--
 app/controllers/bookmarks_controller.rb        | 6 +++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/app/controllers/api/v1/bookmarks_controller.rb b/app/controllers/api/v1/bookmarks_controller.rb
index 02647bd3fd..2888beba22 100644
--- a/app/controllers/api/v1/bookmarks_controller.rb
+++ b/app/controllers/api/v1/bookmarks_controller.rb
@@ -22,8 +22,9 @@ def create
       return
     end
 
-    bookmark = Bookmark.where(user: current_user, reply: reply, post: reply.post, type: "reply_bookmark").first_or_initialize
-    unless bookmark.update(params.permit(:name, :public))
+    bookmark = Bookmark.where(user: current_user, reply: reply, type: "reply_bookmark").first_or_initialize
+    params[:post_id] = reply.post_id
+    unless bookmark.update(params.permit(:name, :public, :post_id))
       error = { message: 'Bookmark could not be created.' }
       render json: { errors: [error] }, status: :unprocessable_entity
       return
diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb
index 4f7cac1d7e..fcbb627fd8 100644
--- a/app/controllers/bookmarks_controller.rb
+++ b/app/controllers/bookmarks_controller.rb
@@ -50,10 +50,10 @@ def create
       return redirect_to posts_path
     end
 
-    bookmark = Bookmark.where(reply_id: @reply.id, user_id: current_user.id, post_id: @reply.post.id,
-      type: 'reply_bookmark',).first_or_initialize
+    bookmark = Bookmark.where(reply_id: @reply.id, user_id: current_user.id, type: 'reply_bookmark').first_or_initialize
     if bookmark.new_record?
-      bookmark.update!(params.permit(:name, :public))
+      params[:post_id] = @reply.post_id
+      bookmark.update!(params.permit(:name, :public, :post_id))
       flash[:success] = "Bookmark added."
     else
       flash[:error] = "Bookmark already exists."

From a10cd84bf9baff8652ea681c8cc4011e176e2fdf Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 19:46:04 -0300
Subject: [PATCH 23/27] Move more logic into before_action

---
 .../api/v1/bookmarks_controller.rb            | 20 ++++------------
 app/controllers/bookmarks_controller.rb       | 24 +++++++++----------
 spec/controllers/bookmarks/destroy_spec.rb    |  2 +-
 3 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/app/controllers/api/v1/bookmarks_controller.rb b/app/controllers/api/v1/bookmarks_controller.rb
index 2888beba22..ea47d4c180 100644
--- a/app/controllers/api/v1/bookmarks_controller.rb
+++ b/app/controllers/api/v1/bookmarks_controller.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 class Api::V1::BookmarksController < Api::ApiController
   before_action :login_required
-  before_action :find_bookmark, except: :create
+  before_action :bookmark_ownership_required, except: :create
 
   resource_description do
     description 'Viewing and modifying bookmarks'
@@ -17,10 +17,7 @@ class Api::V1::BookmarksController < Api::ApiController
   error 422, "Invalid parameters provided"
   def create
     return unless (reply = find_object(Reply, param: :reply_id))
-    unless reply.post.visible_to?(current_user)
-      access_denied
-      return
-    end
+    access_denied and return unless reply.post.visible_to?(current_user)
 
     bookmark = Bookmark.where(user: current_user, reply: reply, type: "reply_bookmark").first_or_initialize
     params[:post_id] = reply.post_id
@@ -42,11 +39,6 @@ def create
   error 404, "Bookmark not found"
   error 422, "Invalid parameters provided"
   def update
-    if @bookmark.user.id != current_user.try(:id)
-      access_denied
-      return
-    end
-
     unless @bookmark.update(params.permit(:name, :public))
       error = { message: 'Bookmark could not be updated.' }
       render json: { errors: [error] }, status: :unprocessable_entity
@@ -63,11 +55,6 @@ def update
   error 404, "Bookmark not found"
   error 422, "Invalid parameters provided"
   def destroy
-    if @bookmark.user.id != current_user.try(:id)
-      access_denied
-      return
-    end
-
     unless @bookmark.destroy
       error = { message: 'Bookmark could not be removed.' }
       render json: { errors: [error] }, status: :unprocessable_entity
@@ -79,8 +66,9 @@ def destroy
 
   private
 
-  def find_bookmark
+  def bookmark_ownership_required
     return unless (@bookmark = find_object(Bookmark))
     access_denied unless @bookmark.visible_to?(current_user)
+    access_denied unless @bookmark.user.id == current_user.try(:id)
   end
 end
diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb
index fcbb627fd8..f1433e57fa 100644
--- a/app/controllers/bookmarks_controller.rb
+++ b/app/controllers/bookmarks_controller.rb
@@ -2,8 +2,8 @@
 require 'will_paginate/array'
 
 class BookmarksController < ApplicationController
-  before_action :login_required, except: [:search]
-  before_action :find_model, only: [:destroy]
+  before_action :login_required, except: :search
+  before_action :bookmark_ownership_required, only: :destroy
 
   def search
     @page_title = 'Search Bookmarks'
@@ -63,12 +63,6 @@ def create
   end
 
   def destroy
-    @reply = @bookmark.reply
-    unless @bookmark.user.id == current_user.try(:id)
-      flash[:error] = "You do not have permission to remove this bookmark."
-      redirect_back fallback_location: reply_path(@reply, anchor: "reply-#{@reply.id}") and return
-    end
-
     begin
       @bookmark.destroy!
     rescue ActiveRecord::RecordNotDestroyed => e
@@ -82,11 +76,17 @@ def destroy
 
   private
 
-  def find_model
+  def bookmark_ownership_required
     @bookmark = Bookmark.find_by_id(params[:id])
-    return if @bookmark&.visible_to?(current_user)
+    unless @bookmark&.visible_to?(current_user)
+      flash[:error] = "Bookmark could not be found."
+      redirect_to posts_path and return
+    end
 
-    flash[:error] = "Bookmark could not be found."
-    redirect_to posts_path and return
+    @reply = @bookmark.reply
+    return if @bookmark.user.id == current_user.try(:id)
+
+    flash[:error] = "You do not have permission to perform this action."
+    redirect_back fallback_location: reply_path(@reply, anchor: "reply-#{@reply.id}")
   end
 end
diff --git a/spec/controllers/bookmarks/destroy_spec.rb b/spec/controllers/bookmarks/destroy_spec.rb
index 1cd8cb79d9..1db3e2dcbd 100644
--- a/spec/controllers/bookmarks/destroy_spec.rb
+++ b/spec/controllers/bookmarks/destroy_spec.rb
@@ -28,7 +28,7 @@
     login
     delete :destroy, params: { id: bookmark.id }
     expect(response).to redirect_to(reply_url(reply, anchor: "reply-#{reply.id}"))
-    expect(flash[:error]).to eq("You do not have permission to remove this bookmark.")
+    expect(flash[:error]).to eq("You do not have permission to perform this action.")
   end
 
   it "succeeds for bookmark owner" do

From 39d24a3ccbd5da6dd3f62f2699bbccb2eed192a4 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 19:53:09 -0300
Subject: [PATCH 24/27] Rename spec scenario

---
 spec/controllers/api/v1/bookmarks_controller_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/controllers/api/v1/bookmarks_controller_spec.rb b/spec/controllers/api/v1/bookmarks_controller_spec.rb
index 47ff2c3f32..b4c252f952 100644
--- a/spec/controllers/api/v1/bookmarks_controller_spec.rb
+++ b/spec/controllers/api/v1/bookmarks_controller_spec.rb
@@ -134,7 +134,7 @@
       expect(response.parsed_body['errors'][0]['message']).to eq("Bookmark could not be found.")
     end
 
-    it "requires visible bookmark", :show_in_doc do
+    it "requires visible post", :show_in_doc do
       api_login
       bookmark = create(:bookmark)
       bookmark.post.update!(privacy: :private)

From 1316cd9d80a6783f613ab1fcadc662d173c0257f Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 19:55:36 -0300
Subject: [PATCH 25/27] Rename spec scenarios

---
 spec/controllers/bookmarks/create_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/controllers/bookmarks/create_spec.rb b/spec/controllers/bookmarks/create_spec.rb
index d686f24c05..55cf077620 100644
--- a/spec/controllers/bookmarks/create_spec.rb
+++ b/spec/controllers/bookmarks/create_spec.rb
@@ -35,7 +35,7 @@
     expect(bookmark.name).to be_nil
   end
 
-  it "succeeds with name" do
+  it "succeeds with name param" do
     login
     post :create, params: { at_id: reply.id, name: "new bookmark" }
     expect(response).to redirect_to(reply_url(reply, anchor: "reply-#{reply.id}"))
@@ -45,7 +45,7 @@
     expect(bookmark.public).to be false
   end
 
-  it "succeeds with public" do
+  it "succeeds with public param" do
     login
     post :create, params: { at_id: reply.id, public: true }
     expect(response).to redirect_to(reply_url(reply, anchor: "reply-#{reply.id}"))

From 10bfe371885f3e79bac8f133cdee3716903a5803 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Sun, 22 Dec 2024 20:08:10 -0300
Subject: [PATCH 26/27] Add missing return

---
 app/controllers/api/v1/bookmarks_controller.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/controllers/api/v1/bookmarks_controller.rb b/app/controllers/api/v1/bookmarks_controller.rb
index ea47d4c180..87022a3104 100644
--- a/app/controllers/api/v1/bookmarks_controller.rb
+++ b/app/controllers/api/v1/bookmarks_controller.rb
@@ -68,7 +68,7 @@ def destroy
 
   def bookmark_ownership_required
     return unless (@bookmark = find_object(Bookmark))
-    access_denied unless @bookmark.visible_to?(current_user)
+    access_denied and return unless @bookmark.visible_to?(current_user)
     access_denied unless @bookmark.user.id == current_user.try(:id)
   end
 end

From ff5cabcab3a11fd7e9a1b3592062e3c98eaf5891 Mon Sep 17 00:00:00 2001
From: Red Magnos <redmagnos@gmail.com>
Date: Mon, 23 Dec 2024 12:43:37 -0300
Subject: [PATCH 27/27] Use .merge rather than .permit when upserting bookmarks

---
 app/controllers/api/v1/bookmarks_controller.rb | 3 +--
 app/controllers/bookmarks_controller.rb        | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/app/controllers/api/v1/bookmarks_controller.rb b/app/controllers/api/v1/bookmarks_controller.rb
index 87022a3104..0cb065d9ae 100644
--- a/app/controllers/api/v1/bookmarks_controller.rb
+++ b/app/controllers/api/v1/bookmarks_controller.rb
@@ -20,8 +20,7 @@ def create
     access_denied and return unless reply.post.visible_to?(current_user)
 
     bookmark = Bookmark.where(user: current_user, reply: reply, type: "reply_bookmark").first_or_initialize
-    params[:post_id] = reply.post_id
-    unless bookmark.update(params.permit(:name, :public, :post_id))
+    unless bookmark.update(params.permit(:name, :public).merge(post_id: reply.post_id))
       error = { message: 'Bookmark could not be created.' }
       render json: { errors: [error] }, status: :unprocessable_entity
       return
diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb
index f1433e57fa..6a94845975 100644
--- a/app/controllers/bookmarks_controller.rb
+++ b/app/controllers/bookmarks_controller.rb
@@ -52,8 +52,7 @@ def create
 
     bookmark = Bookmark.where(reply_id: @reply.id, user_id: current_user.id, type: 'reply_bookmark').first_or_initialize
     if bookmark.new_record?
-      params[:post_id] = @reply.post_id
-      bookmark.update!(params.permit(:name, :public, :post_id))
+      bookmark.update!(params.permit(:name, :public).merge(post_id: @reply.post_id))
       flash[:success] = "Bookmark added."
     else
       flash[:error] = "Bookmark already exists."