FEATURE: Add bulk action to bookmark #26856

branquinhoaa · 2024-05-02T23:00:26Z

This PR aims to add bulk actions to the user's bookmarks.

After this feature, all users should be able to select multiple bookmarks and perform the actions of "deleting" or "clear reminders" as shown below:

branquinhoaa

Self-review.

app/assets/javascripts/discourse/app/components/bookmark-list.hbs

app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js

branquinhoaa · 2024-05-02T23:03:43Z

app/assets/javascripts/discourse/app/models/bookmark.js

@@ -42,6 +42,18 @@ export default class Bookmark extends RestModel {
    });
  }

+  static bulkOperation(bookmarks, operation) {


Added a method to support bulk operations on bookmarks.

branquinhoaa · 2024-05-02T23:04:02Z

app/assets/javascripts/discourse/app/raw-templates/flat-button.hbr

@@ -1,3 +1,3 @@
-<button class='btn-transparent {{class}}' title='{{i18n "topics.bulk.toggle"}}'>
+<button class='btn-transparent {{class}}' title='{{i18n title}}'>


This was just a minor refactoring to get the title dynamically instead of always getting the hardcoded for topics.

Probably don't need to include this change either now that bookmarks don't use the raw button.

branquinhoaa · 2024-05-02T23:04:33Z

app/assets/javascripts/discourse/tests/acceptance/user-bookmark-bulk-actions-test.js

+    );
+  });
+
+  test("bulk select - clear reminders", async function (assert) {


This is the test I mentioned before; it is currently flakey because of the dialog service.

Yeah, this is tricky. I am not sure it ahs to do with the dialog service, that service should be available at all times. I suspect it has to do with raw handlebars and the fact that here two tests are loading the same endpoint.

Once we move away from handlebars, this should clear. If it doesn't, we can sidestep tihs issue by having just one test that covers both the deleting and the reminder clearing checks.

I've updated the templates, but the test was still flakey, so I combined all the tests into one, as you suggested.

app/controllers/bookmarks_controller.rb

branquinhoaa · 2024-05-02T23:05:06Z

lib/bookmark_reminder_notification_handler.rb

@@ -24,8 +24,6 @@ def send_notification
    end
  end

-  private


I'm using this method to clear the reminders for bookmarks.

branquinhoaa · 2024-05-02T23:05:31Z

spec/requests/bookmarks_controller_spec.rb

  describe "#create" do
+    before { sign_in(current_user) }


I had to move this inside the describe blocks to add tests for signed out users.

pmusaraj

Thanks @branquinhoaa, this is a great start. I left some comments from a first review pass.

I also took it for a spin locally, and it works well. The main comment is regarding raw handlebars templates, we should try and avoid them as much as possible. (I know you're basing this off the existing raw templates, but that's legacy code that we plan to retire in the near future.)

pmusaraj · 2024-05-03T17:03:47Z

app/assets/javascripts/discourse/app/components/bookmark-list.hbs

+              {{#if this.bulkSelectEnabled}}
+                <span class="bulk-select-topics">
+                  {{~#if this.canDoBulkActions}}
+                    {{raw


Can you try to use the Glimmer component directly here? For context, raw handlebars components are legacy elements, we are rtying to move away from them. (There's no way for you to have known this before...)

I suspect this may also help with the dialog test issue.

pmusaraj · 2024-05-03T17:04:09Z

app/assets/javascripts/discourse/app/components/bookmark-list.hbs

+                    }}</button>
+                </span>
+              {{else}}
+                {{raw


Same here, het's try to avoid any raw components.

pmusaraj · 2024-05-03T17:06:46Z

app/assets/javascripts/discourse/tests/acceptance/user-bookmark-bulk-actions-test.js

+    );
+  });
+
+  test("bulk select - clear reminders", async function (assert) {


Yeah, this is tricky. I am not sure it ahs to do with the dialog service, that service should be available at all times. I suspect it has to do with raw handlebars and the fact that here two tests are loading the same endpoint.

Once we move away from handlebars, this should clear. If it doesn't, we can sidestep tihs issue by having just one test that covers both the deleting and the reminder clearing checks.

pmusaraj · 2024-05-03T17:07:10Z

app/assets/javascripts/discourse/tests/acceptance/user-bookmark-bulk-actions-test.js

+  });
+
+  test("bulk select - options", async function (assert) {
+    updateCurrentUser({ moderator: true });


Very minor, but you likely don't ned to set moderator to true here. Regular users should be able to update their own bookmarks.

pmusaraj · 2024-05-03T17:09:28Z

app/assets/javascripts/select-kit/addon/components/bulk-select-bookmarks-dropdown.js

+  },
+
+  showBulkBookmarksActionsModal(operationType, description, successMessage) {
+    this.dialog.yesNoConfirm({


For the "delete bookmark" use case, it may be better to use the delete confirm dialog.

pmusaraj · 2024-05-03T17:11:22Z

spec/requests/bookmarks_controller_spec.rb

+      end
+
+      it "can clear reminder for the given bookmarks" do
+        Guardian.any_instance.stubs(:can_edit?).returns(true)


Instead of stubbing the guardian check, it might be better to run this call as the correct user. And also run it as a second user (which should fail to update the bookmarks for user 1.)

pmusaraj · 2024-05-03T17:11:50Z

spec/requests/bookmarks_controller_spec.rb

+      end
+
+      it "can delete bookmarks" do
+        Guardian.any_instance.stubs(:can_edit?).returns(true)


Same here, given this is a destructire operation, we want to make sure the only right users can do this.

pmusaraj · 2024-05-03T17:14:56Z

lib/bookmarks_bulk_action.rb

+    @operations ||= %w[clear_reminder delete]
+  end
+
+  def self.register_operation(name, &block)


I don't think we need to add an extension point at this time.

pmusaraj · 2024-05-03T17:17:52Z

app/controllers/bookmarks_controller.rb

+    operation =
+      params
+        .require(:operation)
+        .permit(:type, *DiscoursePluginRegistry.permitted_bulk_action_parameters)


I think we can keep the bookmark operation types separate from the topic bulk action operation types here.

pmusaraj · 2024-05-03T17:19:09Z

spec/lib/bookmarks_bulk_action_spec.rb

+    describe "when user can't delete" do
+      it "does NOT delete the bookmarks" do
+        Guardian.any_instance.stubs(:can_edit?).returns(false)
+        Guardian.any_instance.stubs(:can_delete?).returns(false)


Here too we should also have a few tests with the bookmark edit permission.

I checked the bookmark guardian implementation, and it seems that currently, all users who can edit can also delete. So, the tests will be the same. I still added them to avoid future regressions, though.

- Replace raw handlebars components with glimmer components - Remove stubs from rspec tests - Add more test cases - Fix flakey acceptance tests

branquinhoaa · 2024-05-04T17:33:42Z

@pmusaraj Thanks for the review!

I've applied your suggestions and left some comments on them.

Could you take another look? Please let me know if you have anything else in mind.

pmusaraj

Thanks for the updates, a few more comments.

app/assets/javascripts/discourse/app/components/bookmark-list.hbs

app/assets/javascripts/discourse/app/raw-views/bookmark-bulk-select-dropdown.gjs

pmusaraj · 2024-05-06T14:15:48Z

app/assets/javascripts/discourse/app/raw-templates/flat-button.hbr

@@ -1,3 +1,3 @@
-<button class='btn-transparent {{class}}' title='{{i18n "topics.bulk.toggle"}}'>
+<button class='btn-transparent {{class}}' title='{{i18n title}}'>


Probably don't need to include this change either now that bookmarks don't use the raw button.

app/assets/javascripts/select-kit/addon/components/bulk-select-bookmarks-dropdown.js

pmusaraj · 2024-05-06T14:20:38Z

app/assets/javascripts/select-kit/addon/components/bulk-select-bookmarks-dropdown.js

+            })
+              .then(() => {
+                this.router.refresh();
+                this.bulkSelectHelper.toggleBulkSelect();


I'm not sure we need to toggle the bulk select state here. Maybe the user wants to continue bulk selecting and editing other bookmarks?

It seems that the toggle is not necessary since the router refresh is already cleaning up the selection.

In terms of user experience, however, I can't see a specific case where keeping the selection would be helpful for the current "bookmark's bulk operations."

If the user deletes the bookmark, he can't clean up the reminders, and if they spend effort cleaning the reminders, I don't think they will want to delete the same bookmarks in a subsequent operation.

Are you thinking about a specific workflow? Maybe I'm missing something; I will be happy to fix this; just thought it could be something to think about and discuss :)

Yes, if you have say 20 bookmarks, you might want to:

select 5, clear reminders

select 2, delete

continue with more selections

Ooh, sorry for the confusion! I misunderstood your suggestion. I thought you wanted to keep the checkboxes selected, which is not what you are suggesting 😅
So, this is what we have at this point - I guess it matches your initial idea:

spec/lib/bookmarks_bulk_action_spec.rb

app/assets/javascripts/discourse/tests/acceptance/user-bookmark-bulk-actions-test.js

branquinhoaa · 2024-05-07T01:59:40Z

Hey @pmusaraj, thank you again for your time here; I really appreciate it.

I added some notes for context - just to clarify some of the decisions I made, but please let me know if you disagree 😃

pmusaraj

A few minor cleanups, but otherwise this is looking great, thanks!

pmusaraj · 2024-05-07T13:56:45Z

app/assets/javascripts/discourse/app/components/bookmark-list.js

  _removeBookmarkFromList(bookmark) {
    this.content.removeObject(bookmark);
  },
+
+  _toggleBookmark(target, bookmark, isSelectingRange) {


I think this should be named _toggleSelection, because it doesn't change the bookmark status, it just changes the selected checkboxes.

pmusaraj · 2024-05-07T14:06:59Z

app/assets/javascripts/discourse/app/raw-templates/bookmark-bulk-select-dropdown.hbr

@@ -0,0 +1 @@
+{{{view.html}}}


This can be dropped as well?

pmusaraj · 2024-05-07T14:11:07Z

lib/bookmarks_bulk_action.rb

+        BookmarkReminderNotificationHandler.new(b).clear_reminder
+        @changed_ids << b.id
+      else
+        raise Discourse::InvalidAccess.new


Ah, that makes sense, we are doing the same for deletes. I think you're right to keep the consistency. Maybe we move the check inside of BookmarkReminderNotificationHandler.new(b).clear_reminder so that individual reminder clearances also get the access check?

pmusaraj · 2024-05-07T14:24:46Z

app/assets/javascripts/select-kit/addon/components/bulk-select-bookmarks-dropdown.js

+  _customActions[name] = customAction;
+}
+
+export function addBulkDropdownButton(opts) {


I don't think we need this for bookmarks. In the topics list, this is linked to a plugin API endpoint that lets plugins add their own buttons and actions.

- Remove unused file, unused functions, rename functions

branquinhoaa · 2024-05-08T02:24:29Z

Hey @pmusaraj! These are done 😄 let me know what you think about the update for BookmarkReminderNotificationHandler

pmusaraj · 2024-05-17T14:32:18Z

@branquinhoaa I ran the specs here multiple times today (because I wanted to merge the PR), but they fail regularly, at the same place. 🤔

branquinhoaa · 2024-05-17T14:39:03Z

@pmusaraj I will check today!

branquinhoaa · 2024-05-17T23:44:27Z

Hey @pmusaraj, I have checked my commit where it started failing, and I didn't find anything that could justify the error, so I'm updating my branch with the latest changes pushed to main to see if this could help.

I have enabled Github actions on my fork to check if tests are passing. It is taking a while because of my GitHub plan. I will let you know the results once it is done 👍

Do you know if there is an easier way to run these tests locally? I saw quite a bit of setup on the CI file.

pmusaraj · 2024-05-22T15:29:30Z

Sorry for my delay here @branquinhoaa, I think we're good to merge this now and keep an eye on flakey tests. (The license ❌ is unrelated, it should be fixed in main already.)

Thanks for your work! LGTM!

I'll let you click the Merge button, you should have that power now 😄

branquinhoaa · 2024-05-22T15:51:12Z

Yeey!! I just merged my first PR into Discourse's codebase 🥳 thank you @pmusaraj

FEATURE: add bulk actions to user's bookmarks

1f764a8

github-actions bot added the i18n PRs which update English locale files or i18n related code label May 2, 2024

implement API and integrate with FE

57ecf3c

branquinhoaa force-pushed the add-bulk-action-to-bookmark branch from c12b2c6 to 57ecf3c Compare May 2, 2024 23:01

branquinhoaa commented May 2, 2024

View reviewed changes

branquinhoaa marked this pull request as ready for review May 2, 2024 23:06

pmusaraj reviewed May 3, 2024

View reviewed changes

Refactor tests and glimmer components

7d50c44

- Replace raw handlebars components with glimmer components - Remove stubs from rspec tests - Add more test cases - Fix flakey acceptance tests

pmusaraj reviewed May 6, 2024

View reviewed changes

Update tests and clean up code.

d1f7b6a

branquinhoaa changed the title ~~Add bulk action to bookmark~~ FEATURE: Add bulk action to bookmark May 7, 2024

pmusaraj approved these changes May 7, 2024

View reviewed changes

Clean up code

3613522

- Remove unused file, unused functions, rename functions

Merge branch 'main' into add-bulk-action-to-bookmark

1cfce0b

branquinhoaa merged commit b0d95c8 into discourse:main May 22, 2024
14 of 16 checks passed

		@@ -1,3 +1,3 @@
		<button class='btn-transparent {{class}}' title='{{i18n "topics.bulk.toggle"}}'>
		<button class='btn-transparent {{class}}' title='{{i18n title}}'>

@@ @@ -24,8 +24,6 @@ def send_notification @@
                   end
                 end
-                private

		@@ -0,0 +1 @@
		{{{view.html}}}

FEATURE: Add bulk action to bookmark #26856

FEATURE: Add bulk action to bookmark #26856

Conversation

branquinhoaa commented May 2, 2024

branquinhoaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmusaraj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branquinhoaa commented May 4, 2024

pmusaraj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branquinhoaa commented May 7, 2024

pmusaraj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branquinhoaa commented May 8, 2024

pmusaraj commented May 17, 2024

branquinhoaa commented May 17, 2024

branquinhoaa commented May 17, 2024

pmusaraj commented May 22, 2024 • edited

branquinhoaa commented May 22, 2024

pmusaraj commented May 22, 2024 •

edited