Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add batch actions top bar element #2233

Closed
wants to merge 1 commit into from
Closed

Conversation

rocodes
Copy link
Contributor

@rocodes rocodes commented Sep 17, 2024

Status

Ready for review but presumes #2230 which should be reviewed first, will rebase once that's merged

Description

Add Top Bar UI element nested inside main layout, which will hold "Delete Sources" toolbar button (and eventually, other toolbar buttons). UI-only changes towards, #2160

Test Plan

  • Visual review
  • CI

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:

  • I have updated the AppArmor profile
  • No update to the AppArmor profile is required for these changes
  • I don't know and would appreciate guidance

If these changes modify the database schema, you should include a database migration. Please check as applicable:

  • I have written a migration and upgraded a test database based on main and confirmed that the migration is self-contained and applies cleanly
  • I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
  • I need help writing a database migration
  • No database schema changes are needed

@rocodes rocodes changed the title Add bulk actions top bar element Add batch actions top bar element Sep 17, 2024
@rocodes rocodes added this to the 0.14.0 milestone Sep 17, 2024
@rocodes rocodes force-pushed the add-bulk-actions-pane branch 2 times, most recently from 0236763 to f9640a6 Compare September 18, 2024 08:33
@rocodes rocodes force-pushed the add-bulk-actions-pane branch from f9640a6 to b83c526 Compare September 18, 2024 08:41
@cfm cfm force-pushed the add-bulk-actions-pane branch from b83c526 to 298eb13 Compare October 2, 2024 00:12
@cfm
Copy link
Member

cfm commented Oct 2, 2024

Rebased from main to kick the tires. Oddly, GitHub still shows a conflict with main, but we can resolve that in the course of finishing and testing this work.

@rocodes rocodes force-pushed the add-bulk-actions-pane branch 2 times, most recently from 1b0942d to 2ad8e91 Compare October 3, 2024 13:18
@rocodes rocodes marked this pull request as ready for review October 3, 2024 13:49
@rocodes rocodes requested a review from a team as a code owner October 3, 2024 13:49
MainView layout to QVBoxLayout and add inner horizontal container to
accommodate inner top bar.

Update strings
@rocodes rocodes force-pushed the add-bulk-actions-pane branch from 2ad8e91 to c8aecc3 Compare October 3, 2024 16:14
Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review. Since this adds a nonfunctional widget—the Delete Sources button is not wired up to anything—I'll review for real along with #2252 once that's ready.

Comment on lines +518 to +524
def delete_multiple_sources(self) -> None:
"""
Requires logged-in session. Delete currently-selected sources.
"""
logger.debug("delete_multiple_sources triggered")
if self.controller.api is None:
self.controller.on_action_requiring_login()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can eliminate the conditional with:

Suggested change
def delete_multiple_sources(self) -> None:
"""
Requires logged-in session. Delete currently-selected sources.
"""
logger.debug("delete_multiple_sources triggered")
if self.controller.api is None:
self.controller.on_action_requiring_login()
@login_required # from securedrop_client.logic import login_required
def delete_multiple_sources(self) -> None:
"""
Delete currently-selected sources.
"""
logger.debug("delete_multiple_sources triggered")

Comment on lines +478 to +487
A toolbar that contains batch actions (actions that target multiple
sources in the ConversationView, and therefore don't belong in the
individual conversation menu). Currently, this widget will hold the
"Delete Sources" (batch-delete) action.

For user-facing naming consistency, these items won't be called
"batch/bulk <delete>", but simply "<verb> <noun>s" (eg "Delete Sources"), where
the original nomenclature comes from the individual Source overflow QAction menu
items. Each item may have a tooltip, visible on hover, that provides a more
lengthy explanation (e.g., "Delete multiple source accounts").
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the local style guide for "here's how to use this section of the UI". I'll note for the record that I continue to think that "batch" is an implementation detail of what is really just a "bulk" operation or an operation over multiple objects—not all of which will batch the individual operations or requests. I'm fine with whatever terminology we adopt as long as we don't wind up confused by it.

@cfm cfm self-assigned this Oct 4, 2024
@cfm cfm added the blocked label Oct 4, 2024
@cfm
Copy link
Member

cfm commented Oct 25, 2024

I'm inclined to merge c8aecc3 as part of #2252. I think the separation is too minimal by now to be worth the rebase from here. What do you think, @rocodes?

@rocodes
Copy link
Contributor Author

rocodes commented Oct 25, 2024

yeah I agree - might as well close it and merge them together. I was initially trying to split up the work into readily reviewable chunks so that people could tag team as needed, but might as well just do all in #2252.

@rocodes
Copy link
Contributor Author

rocodes commented Oct 25, 2024

Closing in favour of #2252

@rocodes rocodes closed this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants