7778 - Bulk delete redesign #7846

latin-panda · 2022-10-17T04:05:50Z

Description

Border left indicator was moved from .content-row a to .content-row as side effect of separating click scope from anchors and checkboxes, all tabs' lists using .content-row should work fine same as original code.

The action bar's buttons related to bulk delete and select mode (select all, deselect all, etc) have been removed from here and now the multiselect bar component will handle some of these. The action bar will eventually disappear as part of the UI evolution ("FAB", "More options" tasks)
Store's selected report has been split in selectedReport (a report selected to open/view its content) and selectedReports (a collection of reports selected for bulk operation). This is because the user can open to view a report while the select mode is active on mobile devices.
The select mode is now calculated in the Report's component, it's based on the last state of the store.
The modal displaying "Bulk deletion has completed. Click complete to refresh the page" will not have the cancel button anymore because it was doing the same as the "complete" button, it's considered redundant.
Adding default values to navigation component properties to avoid angular exception on change detection.
A telemetry record is saved when the user bulk delete, counting number of docs deleted.
The protractor test submit-delivery-form.specs.js was already migrated, so I removed it and updated the PROTRACTOR_WDIO_MIGRATION_STATE.md.
We've decided to not fully support responsive UI, details here.

Screenshots of feature.

CHT_Docs

#7778

Code review checklist

Readable: Concise, well named, follows the style guide, documented if necessary.
Documented: Configuration and user documentation on cht-docs
Tested: Unit and/or e2e where appropriate
Internationalised: All user facing text
Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

…bulk-delete-redesign

…ts for bulk upload

…bulk-delete-redesign

latin-panda · 2022-10-19T09:29:52Z

Hi @dianabarsan can you please review?

@tatilepizs can you please review too and let me know if any other coverage is needed?

Thanks for you time!

latin-panda · 2022-10-28T10:25:39Z

Hi @tatilepizs I've updated the tests, can you please have a look again? Thanks!

Nb. I've updated the ticket and the PR's description with latest screenshots and decisions about the feature.

tatilepizs

Is still looking good to me 😄
Thanks @latin-panda !

dianabarsan

Excellent work! I really like the changes in effects and action names, it makes it much clearer to understand compared to the old version and makes the isMobile required fewer times. Great!

I did add some minor comments and an e2e test request, if possible.
Thanks!!

webapp/tests/karma/ts/modules/reports/reports.component.spec.ts

webapp/src/ts/effects/reports.effects.ts

webapp/src/ts/modules/reports/report-route.guard.provider.ts

webapp/src/ts/modules/reports/reports.component.ts

webapp/src/ts/reducers/reports.ts

webapp/tests/karma/ts/effects/reports.effects.spec.ts

dianabarsan · 2022-10-31T17:38:24Z

tests/e2e/default/reports/bulk-delete.wdio-spec.js

@@ -1,6 +1,6 @@
 const utils = require('../../../utils');
 const commonElements = require('../../../page-objects/default/common/common.wdio.page');
-const reports = require('../../../page-objects/default/reports/reports.wdio.page');
+const reportsPage = require('../../../page-objects/default/reports/reports.wdio.page');
 const loginPage = require('../../../page-objects/default/login/login.wdio.page');


Since the functionality differs between mobile and desktop view and we have a mobile e2e suite, please add an e2e test that checks bulk deletion for mobile as well.

Good idea, I forgot about the mobile e2e, I'm doing this one in a separate commit for easy review

I looked at this and had a call with QA. The mobile e2e suite is running with protractor, which QA's current target is to migrate everything and remove protractor, because of that we won't be creating more protractor tests.

They have in their very near future plans to work on the mobile e2e suite, which there are some ideas on how to do it, apparently something that Bede started as proof of concept, they will retake and explore based on some ideas from Gareth, like resizing the browser's screen to simulate mobile.

They plan to automate what's in the automation board.

So, we agree that I will help them setting the base for the wdio mobile suite, specifically default config, and have them involve while I do it and automate this case scenario of bulk delete. I've created a new ticket since I don't want to make this PR bigger than it is, this is what I'll be working tomorrow and Friday... and when I come back from my time off.

CC: @tatilepizs

latin-panda · 2022-11-01T16:45:50Z

@dianabarsan I've resolved the feedback, can you please have a look again? Thank you for your time :)

dianabarsan · 2022-11-02T09:06:04Z

I'm having a look at this on desktop and noticing that, when I have any reports selected, and I click on a report content, nothing happens. From a UX perspective, is this the desired result? I find it's pretty confusing for the click to do nothing.

Kazam_screencast_00003.mp4

Maybe it is possible that, when in select mode, clicking on the whole report could set the report as selected?
I'm noticing that in mobile view, clicking on the report body, even if I have other reports selected, does redirect me to report detail page.

dianabarsan · 2022-11-02T09:10:13Z

I have a pretty large database and I'm trying to select and deselect all reports and I'm not really understanding this interaction. If I select all, how can I deselect all afterwards?

Kazam_screencast_00005.mp4

dianabarsan

I'm also noticing that reports list infinite scroll is no longer working. Even if I have a long list of reports, scrolling down no longer loads extra reports.

dianabarsan · 2022-11-02T09:30:09Z

If I'm throttling my network and selecting reports, I see "unknown sender" displayed instead of the summary that already exists in the RHS. Is this expected?

Kazam_screencast_00006.mp4

latin-panda · 2022-11-02T09:51:58Z

I'm having a look at this on desktop and noticing that, when I have any reports selected, and I click on a report content, nothing happens. From a UX perspective, is this the desired result? I find it's pretty confusing for the click to do nothing.

Yes that's the expected behaviour in desktop, if the user wants to see the reports details then they should click on it to expand.

Maybe it is possible that, when in select mode, clicking on the whole report could set the report as selected?

I will try and report back.

If I'm throttling my network and selecting reports, I see "unknown sender" displayed instead of the summary that already exists in the RHS. Is this expected?

The other I reported an issue for that.

I'm also noticing that reports list infinite scroll is no longer working. Even if I have a long list of reports, scrolling down no longer loads extra reports.

Oh it's because the element that scrolls is another one, so ScrollLoaderProvider won't fire... I thing that scroll thing should be a directive so it's easier to discover... anyways, I won't refactor it too much in this PR, will just make it work for now.

I have a pretty large database and I'm trying to select and deselect all reports and I'm not really understanding this interaction. If I select all, how can I deselect all afterwards?

I guess the reportList in store doesn't have the full quantity, I'll think an alternative.

latin-panda · 2022-11-02T18:02:31Z

@dianabarsan Thanks a lot for your feedback!! You can find the commits below:

infinit scroll: commit
Selecting report when clicking in the row while in select mode: commit
Select all when too many reports: commit

I had a call with @n-orlowski and we reviewed the select all behaviour:

When the select all checkbox has a minus, and the user clicks on it
- it should select all (up to 500 reports).
When that checkbox is blue and checked, and the user clicks on it
- it should deselect all.
If the user has too many reports (+500) then select all
- it will only select up to 500 same as in master,
- and the select all checkbox is blue and checked, and the "clear selection" is available.
If the user scrolls more than +500 in the LHS, and "selects all" and then selects some more (for example 505 selected) then that's okay for now, same as master.

Tomorrow my morning I'm going to update the bulk delete ticket with this information and open a new ticket so we can evaluate the behaviour of "select all" when there's more than +500 reports (Edit: Ticket). cc: @tatilepizs

Video this branch vs demo instance (3.17)
https://user-images.githubusercontent.com/66472237/199562802-cc784932-813a-4dbc-9da5-2d9a108fd805.mov

latin-panda · 2022-11-03T03:43:05Z

I've updated this work's ticket with relevant information discussed yesterday.
And I've created the ticket to Improve experience of "select all" when there are +500 reports

latin-panda · 2022-11-03T08:27:40Z

@dianabarsan I think it's ready for another 👀 Thanks!

dianabarsan

Awesome. Thanks a lot for addressing all the feedback! I just have one tiny question left inline.

webapp/src/ts/modules/reports/reports.component.ts

…bulk-delete-redesign � Conflicts: � webapp/src/css/variables.less

…bulk-delete-redesign

latin-panda added 27 commits October 3, 2022 18:33

reports select mode refactor

1037e5d

Merge branch 'master' of https://github.com/medic/cht-core into 7778-…

d387f1a

…bulk-delete-redesign

reports select mode refactor part 2

412a633

reports select mode refactor part 3

f5e55fa

fixing styles for desktop

99b9135

fixing select behaviour

8d36f9d

better code and fixing bugs in select

4c51e12

Split single select to view report content from multi select of repor…

c8915d8

…ts for bulk upload

select experience in mobile and styles

764c192

Fixing mobile select all and deselect

cac545c

Fixing mobile search when deselecting reports

8b7cf94

Adding telemetry to bulk delete

a35b5c7

Merge branch 'master' of https://github.com/medic/cht-core into 7778-…

0898adb

…bulk-delete-redesign

reverting change in api/package-lock

6482ae3

Polishing part 1

e614ead

Polishing part 2: reverting selectMode

7fd017c

fixing unit tests part 1

a5232d6

fixing unit tests part 2

708df7d

Merge branch 'master' of https://github.com/medic/cht-core into 7778-…

79e0ed7

…bulk-delete-redesign

bulk delete e2e

37f1ac5

Fixing flaky e2e

57a2f0a

removing protractor e2e that was already migrated.

1b7dc22

waiting for loaders

0a543f3

making tests more stable

2fa78ae

Temp

f21936f

reverting temp

a9f2f77

adding more unit tests

a7dc951

latin-panda requested review from dianabarsan and tatilepizs October 19, 2022 09:26

latin-panda requested a review from tatilepizs October 28, 2022 10:23

latin-panda marked this pull request as ready for review October 28, 2022 11:32

tatilepizs approved these changes Oct 28, 2022

View reviewed changes

dianabarsan reviewed Oct 31, 2022

View reviewed changes

Feedback part 1

862dc2e

latin-panda requested a review from dianabarsan November 1, 2022 16:45

dianabarsan requested changes Nov 2, 2022

View reviewed changes

latin-panda added 3 commits November 2, 2022 20:46

Feedback: infinite scroll

4bcabb7

Feedback: desktop on select mode - click on row selects.

402750c

Feedback: select all when too many reports

5ece2f8

latin-panda mentioned this pull request Nov 3, 2022

Improve experience of "select all" when there are +500 reports #7883

Open

tiny improvement for multiselect mobile

8f03b18

latin-panda requested a review from dianabarsan November 3, 2022 08:27

dianabarsan approved these changes Nov 3, 2022

View reviewed changes

webapp/src/ts/modules/reports/reports.component.ts Outdated Show resolved Hide resolved

Feedback report summary

d4b6607

latin-panda mentioned this pull request Nov 4, 2022

Bulk delete of reports redesign #7778

Closed

latin-panda added 3 commits November 30, 2022 15:25

Fixing issue

010fe63

Merge branch 'master' of https://github.com/medic/cht-core into 7778-…

f07e4bd

…bulk-delete-redesign � Conflicts: � webapp/src/css/variables.less

Merge branch 'master' of https://github.com/medic/cht-core into 7778-…

f204fbd

…bulk-delete-redesign

latin-panda merged commit 6042a8e into master Dec 2, 2022

latin-panda deleted the 7778-bulk-delete-redesign branch December 2, 2022 01:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7778 - Bulk delete redesign #7846

7778 - Bulk delete redesign #7846

latin-panda commented Oct 17, 2022 •

edited

Loading

latin-panda commented Oct 19, 2022

latin-panda commented Oct 28, 2022 •

edited

Loading

tatilepizs left a comment

dianabarsan left a comment

dianabarsan Oct 31, 2022

latin-panda Nov 1, 2022

latin-panda Nov 1, 2022

latin-panda commented Nov 1, 2022

dianabarsan commented Nov 2, 2022 •

edited

Loading

dianabarsan commented Nov 2, 2022 •

edited

Loading

dianabarsan left a comment

dianabarsan commented Nov 2, 2022

latin-panda commented Nov 2, 2022

latin-panda commented Nov 2, 2022 •

edited

Loading

latin-panda commented Nov 3, 2022

latin-panda commented Nov 3, 2022

dianabarsan left a comment

7778 - Bulk delete redesign #7846

7778 - Bulk delete redesign #7846

Conversation

latin-panda commented Oct 17, 2022 • edited Loading

Description

Code review checklist

License

latin-panda commented Oct 19, 2022

latin-panda commented Oct 28, 2022 • edited Loading

tatilepizs left a comment

Choose a reason for hiding this comment

dianabarsan left a comment

Choose a reason for hiding this comment

dianabarsan Oct 31, 2022

Choose a reason for hiding this comment

latin-panda Nov 1, 2022

Choose a reason for hiding this comment

latin-panda Nov 1, 2022

Choose a reason for hiding this comment

latin-panda commented Nov 1, 2022

dianabarsan commented Nov 2, 2022 • edited Loading

dianabarsan commented Nov 2, 2022 • edited Loading

dianabarsan left a comment

Choose a reason for hiding this comment

dianabarsan commented Nov 2, 2022

latin-panda commented Nov 2, 2022

latin-panda commented Nov 2, 2022 • edited Loading

latin-panda commented Nov 3, 2022

latin-panda commented Nov 3, 2022

dianabarsan left a comment

Choose a reason for hiding this comment

latin-panda commented Oct 17, 2022 •

edited

Loading

latin-panda commented Oct 28, 2022 •

edited

Loading

dianabarsan commented Nov 2, 2022 •

edited

Loading

dianabarsan commented Nov 2, 2022 •

edited

Loading

latin-panda commented Nov 2, 2022 •

edited

Loading