From 16799da580a925b334eb623d8f0e0b96e8175dc7 Mon Sep 17 00:00:00 2001
From: Martin Brennan <mjrbrennan@gmail.com>
Date: Tue, 17 Mar 2020 15:15:22 +1000
Subject: [PATCH] FIX: Improve bookmark modal on mobile and bookmark sync rake
 task (#9221)

* Improve the bookmark mobile on modal so it doesn't go all the way to the edge and the custom datetime input is easier to use
* Improve the rake task for syncing so it does not error for topics that no longer exist and batches 2000 inserts at a time, clearing the array each time
---
 .../discourse/templates/modal/bookmark.hbs    |  4 +--
 .../common/components/bookmark-modal.scss     | 26 ++++++++++++++++++
 lib/tasks/bookmarks.rake                      | 27 ++++++++++---------
 spec/tasks/bookmarks_spec.rb                  | 11 ++++++++
 4 files changed, 54 insertions(+), 14 deletions(-)

diff --git a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs
index 192e10fc2f6..2fa03457326 100644
--- a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs
+++ b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs
@@ -38,14 +38,14 @@
           {{tap-tile icon="ban" text=(I18n "bookmarks.reminders.none") tileId=reminderTypes.NONE activeTile=grid.activeTile onChange=(action "selectReminderType")}}
         {{/tap-tile-grid}}
         {{#if customDateTimeSelected}}
-          <div class="control-group">
+          <div class="control-group custom-date-time-wrap">
             {{d-icon "calendar-alt"}}
             {{date-picker-future
               value=customReminderDate
               onSelect=(action (mut customReminderDate))
             }}
             {{d-icon "far-clock"}}
-            {{input placeholder="--:--" type="time" value=customReminderTime}}
+            {{input placeholder="--:--" type="time" class="time-input" value=customReminderTime}}
           </div>
         {{/if}}
 
diff --git a/app/assets/stylesheets/common/components/bookmark-modal.scss b/app/assets/stylesheets/common/components/bookmark-modal.scss
index 8df4d572035..daad95285aa 100644
--- a/app/assets/stylesheets/common/components/bookmark-modal.scss
+++ b/app/assets/stylesheets/common/components/bookmark-modal.scss
@@ -1,7 +1,11 @@
 .bookmark-with-reminder.modal {
+  .modal-inner-container {
+    max-width: 95%;
+  }
   .modal-body {
     max-width: 410px;
     min-width: 380px;
+    box-sizing: border-box;
 
     .control-label {
       font-weight: 700;
@@ -11,4 +15,26 @@
       width: 100%;
     }
   }
+
+  .custom-date-time-wrap {
+    display: flex;
+    flex-direction: row;
+    align-items: baseline;
+
+    .svg-icon {
+      padding: 0 5px;
+    }
+
+    .date-picker-wrapper {
+      flex: 1;
+
+      .date-picker {
+        width: 100%;
+      }
+    }
+
+    .time-input {
+      flex: 1;
+    }
+  }
 }
diff --git a/lib/tasks/bookmarks.rake b/lib/tasks/bookmarks.rake
index d8f99281cc0..db48125b07a 100644
--- a/lib/tasks/bookmarks.rake
+++ b/lib/tasks/bookmarks.rake
@@ -17,6 +17,7 @@ task "bookmarks:sync_to_table", [:sync_limit] => :environment do |_t, args|
     .joins(:post)
     .where(deleted_at: nil)
     .joins('LEFT JOIN bookmarks ON bookmarks.post_id = post_actions.post_id AND bookmarks.user_id = post_actions.user_id')
+    .joins('INNER JOIN topics ON topics.id = posts.topic_id')
     .where('bookmarks.id IS NULL')
 
   # if sync_limit is provided as an argument this will cap
@@ -27,11 +28,13 @@ task "bookmarks:sync_to_table", [:sync_limit] => :environment do |_t, args|
   end
 
   post_action_bookmark_count = post_action_bookmarks.count('post_actions.id')
-  bookmarks_to_create = []
   i = 0
 
-  Bookmark.transaction do
-    post_action_bookmarks.find_each(batch_size: 1000) do |pab|
+  post_action_bookmarks.find_each(batch_size: 2000) do |pab|
+    # clear at start of each batch to only insert X at a time
+    bookmarks_to_create = []
+
+    Bookmark.transaction do
       RakeHelpers.print_status_with_label("Creating post new bookmarks.......", i, post_action_bookmark_count)
       now = Time.zone.now
       bookmarks_to_create << {
@@ -43,16 +46,16 @@ task "bookmarks:sync_to_table", [:sync_limit] => :environment do |_t, args|
       }
 
       i += 1
-    end
 
-    # this will ignore conflicts in the bookmarks table so
-    # if the user already has a post bookmarked in the new way,
-    # then we don't error and keep on truckin'
-    #
-    # we shouldn't have duplicates here at any rate because of
-    # the above LEFT JOIN but best to be safe knowing this
-    # won't blow up
-    Bookmark.insert_all(bookmarks_to_create)
+      # this will ignore conflicts in the bookmarks table so
+      # if the user already has a post bookmarked in the new way,
+      # then we don't error and keep on truckin'
+      #
+      # we shouldn't have duplicates here at any rate because of
+      # the above LEFT JOIN but best to be safe knowing this
+      # won't blow up
+      Bookmark.insert_all(bookmarks_to_create)
+    end
   end
 
   RakeHelpers.print_status_with_label("Bookmark creation complete!            ", i, post_action_bookmark_count)
diff --git a/spec/tasks/bookmarks_spec.rb b/spec/tasks/bookmarks_spec.rb
index 68f8588e0ba..91249c59f89 100644
--- a/spec/tasks/bookmarks_spec.rb
+++ b/spec/tasks/bookmarks_spec.rb
@@ -43,6 +43,17 @@ RSpec.describe "bookmarks tasks" do
     expect(Bookmark.all.count).to eq(1)
   end
 
+  it "skips post actions where the post topic no longer exists and does not error" do
+    post1.topic.delete
+    post1.reload
+    expect { invoke_task }.not_to raise_error
+  end
+
+  it "skips post actions where the post no longer exists and does not error" do
+    post1.delete
+    expect { invoke_task }.not_to raise_error
+  end
+
   def create_post_actions_and_existing_bookmarks
     Fabricate(:post_action, user: user1, post: post1, post_action_type_id: PostActionType.types[:bookmark])
     Fabricate(:post_action, user: user2, post: post2, post_action_type_id: PostActionType.types[:bookmark])