Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

[Library] Possible inconsistencies between deleting history/bookmark items #9949

Closed
BMEduard opened this issue Apr 15, 2020 · 6 comments · Fixed by #10738 or #12630
Closed

[Library] Possible inconsistencies between deleting history/bookmark items #9949

BMEduard opened this issue Apr 15, 2020 · 6 comments · Fixed by #10738 or #12630
Assignees
@BMEduard
Labels
E5 Estimation Point: about 5 days eng:qa:verified QA Verified Feature:Bookmarks Feature:History

Comments

@BMEduard
Copy link
Contributor

BMEduard commented Apr 15, 2020 •
edited by data-sync-user
Loading

After investigating #8648 I have come across some possible discrepancies between deleting history and bookmark items. These are:

  1. Should we allow users to be able to undo the deletion of individual and/or multiple history items, as it is the case for bookmarks?
  2. Should we also require the user to confirm the deletion of a folder/folders by adding a confirmation pop-up if he uses multiselect to delete multiple bookmark items and one (or more) of them happens to be a folder?
    This would make it consistent with the requested change (in [Library] Deleting is inconsistent between history/bookmarks #8648) to add a confirmation pop-up for deleting a folder. As for the confirmation message, it could be something along the lines of “You have selected one or more folders for deletion. Are you sure you want to delete these items?”. This way the user will be unable to delete a folder without having a confirmation pop up first.

@AmyYLee What are your thoughts regarding these two possible unintended inconsistencies?

┆Issue is synchronized with this Jira Task
@BMEduard BMEduard self-assigned this Apr 15, 2020
@github-actions github-actions bot added the needs:triage Issue needs triage label Apr 15, 2020
@BMEduard BMEduard mentioned this issue Apr 15, 2020
Closed
@AmyYLee
Copy link
Collaborator

AmyYLee commented Apr 28, 2020

After investigating #8648 I have come across some possible discrepancies between deleting history and bookmark items. These are:

1. Should we allow users to be able to undo the deletion of individual and/or multiple history items, as it is the case for bookmarks?

2. Should we also require the user to confirm the deletion of a folder/folders by adding a confirmation pop-up if he uses multiselect to delete multiple bookmark items and one (or more) of them happens to be a folder?
   This would make it consistent with the requested change (in #8648) to add a confirmation pop-up for deleting a folder. As for the confirmation message, it could be something along the lines of “You have selected one or more folders for deletion. Are you sure you want to delete these items?”. This way the user will be unable to delete a folder without having a confirmation pop up first.

@AmyYLee What are your thoughts regarding these two possible unintended inconsistencies?

@betsymi @topotropic What are your thoughts for the above?

@betsymi
Copy link

betsymi commented Apr 29, 2020

  1. For deleting an individual history item/several history items, I see no issue with a snackbar where you could undo. For both: History deleted UNDO
  2. Sure, the confirmation dialog can read the following. [Browser name] will delete the selected items. CANCEL DELETE
    2a. I did this action myself and if possible would like to update the string on the snackbar that appears after to Deleting selected folders UNDO

BMEduard pushed a commit to BMEduard/fenix that referenced this issue May 18, 2020
@darkwing
For issue mozilla-mobile#9949 - Inconsistencies between deleting book…
e9cd0d1 
…mark and history items

 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in mozilla-mobile#8648.
@BMEduard BMEduard linked a pull request May 18, 2020 that will close this issue
For issue #9949 - Inconsistencies between deleting bookmark and history items #10738
Merged
1 task
BMEduard pushed a commit to BMEduard/fenix that referenced this issue May 20, 2020
@darkwing
For issue mozilla-mobile#9949 - Inconsistencies between deleting book…
6de92c6 
…mark and history items

 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in mozilla-mobile#8648.
BMEduard pushed a commit to BMEduard/fenix that referenced this issue May 25, 2020
@darkwing
For issue mozilla-mobile#9949 - Inconsistencies between deleting book…
980e09d 
…mark and history items

 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in mozilla-mobile#8648.
BMEduard pushed a commit to BMEduard/fenix that referenced this issue May 27, 2020
@darkwing
For issue mozilla-mobile#9949 - Inconsistencies between deleting book…
57b9ea9 
…mark and history items

 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in mozilla-mobile#8648.
BMEduard pushed a commit to BMEduard/fenix that referenced this issue May 28, 2020
@darkwing
For issue mozilla-mobile#9949 - Bookmarks/History deletion inconsiste…
508bb42 
…ncies

 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in mozilla-mobile#8648.
BMEduard pushed a commit to BMEduard/fenix that referenced this issue Jun 17, 2020
@darkwing
For issue mozilla-mobile#9949 - Bookmarks/History deletion inconsiste…
1e3f0b0 
…ncies

 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in mozilla-mobile#8648.
ekager pushed a commit that referenced this issue Jun 17, 2020
@darkwing @ekager
For issue #9949 - Bookmarks/History deletion inconsistencies
3feab90 
 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in #8648.
@ekager ekager added the eng:qa:needed QA Needed label Jun 17, 2020
@ekager ekager reopened this Jun 17, 2020
@BMEduard BMEduard removed the needs:triage Issue needs triage label Jun 18, 2020
@liuche
Copy link
Contributor

liuche commented Jun 18, 2020

We hit string freeze last Friday, so we can't land any more strings (per our agreement with the L10N team, so that they don't have strings coming in at the last minute).

It's unfortunate because this is such a small change, but we need to back this out and land it after July 20, which is our cut for the Release build (and when we can land strings again).

ekager added a commit that referenced this issue Jun 18, 2020
@ekager
Revert "For issue #9949 - Bookmarks/History deletion inconsistencies"
1c83dc4 
This reverts commit 3feab90.
ekager added a commit that referenced this issue Jun 18, 2020
@ekager
Revert "For issue #9949 - Bookmarks/History deletion inconsistencies"
44ff29b 
This reverts commit 3feab90.
@ekager ekager removed the eng:qa:needed QA Needed label Jun 18, 2020
@ekager
Copy link
Contributor

ekager commented Jun 18, 2020 •
edited
Loading

Reverted #10738, we'll wait and reland it after code freeze

@liuche liuche mentioned this issue Jun 27, 2020
Closed
12 tasks
BMEduard pushed a commit to BMEduard/fenix that referenced this issue Jul 16, 2020
For issue mozilla-mobile#9949 - Bookmarks/History deletion inconsiste…
81b5585 
…ncies

 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in mozilla-mobile#8648.
@BMEduard BMEduard linked a pull request Jul 16, 2020 that will close this issue
For issue #9949 - Bookmarks/History deletion inconsistencies #12630
Merged
5 tasks
BMEduard pushed a commit to BMEduard/fenix that referenced this issue Jul 17, 2020
For issue mozilla-mobile#9949 - Bookmarks/History deletion inconsiste…
a8a6697 
…ncies

 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in mozilla-mobile#8648.
BMEduard pushed a commit to BMEduard/fenix that referenced this issue Jul 17, 2020
For issue mozilla-mobile#9949 - Bookmarks/History deletion inconsiste…
aa3e538 
…ncies

 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in mozilla-mobile#8648.
BMEduard pushed a commit to BMEduard/fenix that referenced this issue Jul 17, 2020
For issue mozilla-mobile#9949 - Bookmarks/History deletion inconsiste…
18afafa 
…ncies

 - Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in mozilla-mobile#8648.
eliserichards pushed a commit that referenced this issue Jul 17, 2020
@BMEduard
For issue #9949 - Bookmarks/History deletion inconsistencies (#12630)
1823fdb 
- Added the undo action for deleting individual history items by creating a new field to the history state containing the id's of the history items that are pending for deletion; This field is used inside the update function from the view to show/hide the items.

 - Added a new check inside the "deleteMulti" method from BookmarkFragment that calls the showRemoveFoldersDialog to prevent the user from being able to delete one or more bookmark folders without being asked for confirmation, as in #8648.

Co-authored-by: Mihai Eduard Badea <[email protected]>
@BranescuMihai BranescuMihai added the E5 Estimation Point: about 5 days label Jul 20, 2020
@liuche liuche mentioned this issue Jul 20, 2020
Closed
9 tasks
@mcarare
Copy link
Contributor

mcarare commented Jul 24, 2020

Reopened for QA.

@mcarare mcarare reopened this Jul 24, 2020
@mcarare mcarare added the eng:qa:needed QA Needed label Jul 24, 2020
@sflorean
Copy link
Contributor

Verified as fixed on 79.1.0-beta.1 with Samsung Galaxy Note 10(Android10). A confirmation dialog is displayed for selected folders, "undo" option is displayed for both history and bookmarks items, previously deleted.

@sflorean sflorean added eng:qa:verified QA Verified and removed eng:qa:needed QA Needed labels Jul 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@BMEduard BMEduard

Labels
E5 Estimation Point: about 5 days eng:qa:verified QA Verified Feature:Bookmarks Feature:History
Projects
None yet
Milestone
No milestone
8 participants
@liuche @BranescuMihai @AmyYLee @BMEduard @ekager @betsymi @mcarare @sflorean