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 warning when all sources are selected for deletion #2300

Merged
merged 5 commits into from
Nov 15, 2024

Conversation

zenmonkeykstop
Copy link
Contributor

Status

Ready for review

Description

Fixes #2298.

Test Plan

  • start client pointed at a dev instance with ~100 sources
  • verify that a single source can be deleted from the button or menu item with the usual text in the dialog
  • select all sources and attempt to delete. verify that a "Notice:..." text has been added, that the continue button starts "Yes, delete all..."
  • add more sources and verify that random counts of sources less than the sum total can be deleted with expected message text.

@zenmonkeykstop zenmonkeykstop requested a review from a team as a code owner November 14, 2024 19:55
@zenmonkeykstop zenmonkeykstop requested a review from cfm November 14, 2024 19:55
@cfm cfm self-assigned this Nov 14, 2024
@cfm cfm added this to the 0.14.0 milestone Nov 14, 2024
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.

  • start client pointed at a dev instance with ~100 sources

Works fine with NUM_SOURCES=10 make dev, for what it's worth....

  • verify that a single source can be deleted from the button or menu item with the usual text in the dialog
  • select all sources and attempt to delete. verify that a "Notice:..." text has been added, that the continue button starts "Yes, delete all..."
  • add more sources and verify that random counts of sources less than the sum total can be deleted with expected message text.

There is the expected race between the list and the dialog after a sync:

  1. Select all sources.
  2. Click Delete Sources.
  3. The "all sources" notice is displayed.
  4. Add a source and wait for sync.
  5. The "all source" notice is still displayed, but no longer accurate.

Should we care? I don't think so. It's an edge case (a new source comes in while the dialog is open) on top of an edge case (all sources selected), but I don't think it's a pathological corner case. The spirit of the dialog (protecting against accidental overdeletion) still holds even if the letter ("all sources") does not.

client/securedrop_client/gui/source/delete/dialog.py Outdated Show resolved Hide resolved
client/securedrop_client/gui/source/delete/dialog.py Outdated Show resolved Hide resolved
client/securedrop_client/logic.py Outdated Show resolved Hide resolved
@zenmonkeykstop zenmonkeykstop force-pushed the 2298-warn-on-select-all branch from e46decd to a6238ba Compare November 15, 2024 01:00
@zenmonkeykstop
Copy link
Contributor Author

Should we care? I don't think so. It's an edge case (a new source comes in while the dialog is open) on top of an edge case (all sources selected), but I don't think it's a pathological corner case. The spirit of the dialog (protecting against accidental overdeletion) still holds even if the letter ("all sources") does not.

Agreed, KISS applies here methinks.

@zenmonkeykstop zenmonkeykstop requested a review from cfm November 15, 2024 01:01
@zenmonkeykstop zenmonkeykstop force-pushed the 2298-warn-on-select-all branch from 882e888 to 62a7d9c Compare November 15, 2024 15:20
@zenmonkeykstop zenmonkeykstop force-pushed the 2298-warn-on-select-all branch from 62a7d9c to 844d801 Compare November 15, 2024 16:24
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.

The test plan still checks out per #2300 (review), and my inline comments are non-blocking. Thanks, @zenmonkeykstop.

super().__init__(show_header=False, dangerous=True)
self.sources = sources
self.source_total = source_total
self.continue_text = "CONTINUE"
Copy link
Member

Choose a reason for hiding this comment

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

Ah! good catch.

client/securedrop_client/logic.py Outdated Show resolved Hide resolved
@cfm cfm added this pull request to the merge queue Nov 15, 2024
Merged via the queue into main with commit 0f02485 Nov 15, 2024
58 checks passed
@cfm cfm deleted the 2298-warn-on-select-all branch November 15, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Update the delete sources dialog text when all sources are selected, to clearly communicate that to the user
2 participants