FIX: Make sure reminder_type is parsed on bookmark update (#9503)

Otherwise we are trying to update the reminder type with a string which often evaluates to 0 (At Desktop) which causes reminders to come through early.

lib/bookmark_manager.rb

@@ -9,7 +9,7 @@ class BookmarkManager
 
   def create(post_id:, name: nil, reminder_type: nil, reminder_at: nil)
     post = Post.unscoped.includes(:topic).find(post_id)
-    reminder_type = Bookmark.reminder_types[reminder_type.to_sym] if reminder_type.present?
+    reminder_type = parse_reminder_type(reminder_type)
 
     raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_see_post?(post)
 
@@ -74,16 +74,20 @@ class BookmarkManager
     raise Discourse::NotFound if bookmark.blank?
     raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_edit?(bookmark)
 
-    if bookmark.errors.any?
-      return add_errors_from(bookmark)
-    end
+    reminder_type = parse_reminder_type(reminder_type)
 
-    bookmark.update(
+    success = bookmark.update(
       name: name,
       reminder_at: reminder_at,
       reminder_type: reminder_type,
       reminder_set_at: Time.zone.now
     )
+
+    if bookmark.errors.any?
+      return add_errors_from(bookmark)
+    end
+
+    success
   end
 
   private
@@ -100,4 +104,9 @@ class BookmarkManager
   def update_topic_user_bookmarked(topic:, bookmarked:)
     TopicUser.change(@user.id, topic, bookmarked: bookmarked)
   end
+
+  def parse_reminder_type(reminder_type)
+    return if reminder_type.blank?
+    reminder_type.is_a?(Integer) ? reminder_type : Bookmark.reminder_types[reminder_type.to_sym]
+  end
 end

spec/lib/bookmark_manager_spec.rb

@@ -181,10 +181,19 @@ RSpec.describe BookmarkManager do
 
     it "saves the time and new reminder type sucessfully" do
       update_bookmark
-       bookmark.reload
-       expect(bookmark.name).to eq(new_name)
-       expect(bookmark.reminder_at).to eq_time(new_reminder_at)
-       expect(bookmark.reminder_type).to eq(new_reminder_type)
+      bookmark.reload
+      expect(bookmark.name).to eq(new_name)
+      expect(bookmark.reminder_at).to eq_time(new_reminder_at)
+      expect(bookmark.reminder_type).to eq(new_reminder_type)
+    end
+
+    context "if the new reminder type is a string" do
+      let(:new_reminder_type) { "custom" }
+      it "is parsed" do
+        update_bookmark
+        bookmark.reload
+        expect(bookmark.reminder_type).to eq(Bookmark.reminder_types[:custom])
+      end
     end
 
     context "if the bookmark is belonging to some other user" do