Give progress indication for export and erasure

[allendav]

Sometimes export and erasure can take a while, e.g. more than 15 seconds even, on slow hosts or when there is a lot to export.

It would be helpful to somehow indicate to the user how export is proceeding. Since the javascript knows what exporter index it is on, we could at least include that as a percentage or something.

[desrosj]

Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.

[dominic_ks]

Hello, I'm new to contributing but picked this up from the Good First Bugs list and started looking at it this morning.

[dominic_ks]

Initial patch to show percentage completion of export or deletion.

[dominic_ks]

I've made a couple of updates to the markup of the export / delete in progress button to contain the percentage progress and also the JS to update this element based on the current exporter or eraser index as a percentage of the total.

I found that if you have the "normal" items registered you largely will see 0%, 33%, 67%, done as it works through posts and comments. Since at present there's no indication of the number of pages that will need to be worked through per item.

There's probably a couple of ways that we could find out the total pages, but perhaps that would be overkill.

[garrett-eclipse]

Speaking with @dominic_ks on Slack there's a few issues with the initial patch he's working to address, updating to indicate his refresh and remove the call to testing until it's in a better state.

[dominic_ks]

Updated patch with fixed indents and also allows for specific progress indicators per job when running multiple exports or deletions at the same time.

[garrett-eclipse]

Refreshed patch to address PHPCS issues and some naming changes

[garrett-eclipse]

Send Export Link w/ Progress

[garrett-eclipse]

Erase Personal Data w/ Progress

[garrett-eclipse]

Thanks for the patch refresh there @dominic_ks it worked really nicely.

In 44264.2.diff I made the following minor changes;

• Fixed PHPCS issues for spacing
• Changed 'delete-progress' to 'erasure-progress' as the tool is for Erasures which don't always involve deletions.
• Changed '$progresCont' to just '$progress' as we want full words for variable naming and I wasn't sure what the 'Cont' meant.

Uploaded some gifs to illustrate the Progress indicators for both the Export and Erasure tools.

This looks great so let's get it in for 5.4 after some more testing. Also just adding needs-design-feedback to ensure the design team like this direction.

Note: For testing unless you expand upon the number of exporters/erasures in your test install it'll always be a quick progress as by default there's not many.

And last question, not sure if it's worth it but am wondering if we should add progress indicators for the admins in the cases they use the admin links for 'Download Personal Data' [Export] and 'Force Erase Personal Data' [Erasure]. These are found by the admin when they hover over the row as row-actions. *I'll post some screens to illustrate.

[garrett-eclipse]

The 'Download Personal Data' row-action which potentially could have the progress indicator

[garrett-eclipse]

The 'Force Erase Personal Data' row-action which potentially could have the progress indicator

[garrett-eclipse]

Added back the new line at the end of privacy-tools.js didn't realize that was part of the JS Coding Standards "There should be a new line at the end of each file." Reference - ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#spacing

[xkon]

Tested the 44264.3.diff​ and seems to be working fine for me and I do agree that the progress should also be indicated on the tables Actions as well if those are used instead by users that have access.

44264.4.diff​ adds the necessary spans for progress indication on the action links also.

[garrett-eclipse]

Minor refresh to remove unneeded extra concat for space before span.

[garrett-eclipse]

Thanks for testing @xkon and the refresh. The inline progress works nicely. In 44264.5.diff I made a minor modification to condense the concatenations and avoid needing the standalone space before the span string, just merged the space into that string.

I'm marking this for commit as it's testing nicely and we've got two maintainers approval here (myself & @xkon).

I will note some minor issues with the UI which are addressed in other tickets.
1) If you're using the row actions and move your cursor away the action vanishes and along with it the progress there. This would be resolved if progress can be made on #48751.
*To illustrate this issue I made a GIF - ​https://i.gyazo.com/49b7f155fb15ce2795f906db417469cf.gif
2) The progress on the button in some cases ends up with the percent split onto two lines so 5 followed by 0% on another line. This makes it hard to read. But is addressed in #46304 which introduces the break-word CSS.
*To illustrate this issue I made a GIF - ​https://i.gyazo.com/fc9c9c7e57b069cc5f67717c92d75fb8.gif

As confirmation of the patch below are some gifs of the various progress triggers;
[Export][Row Action] Download Personal Data & Download Personal Data Again triggers - ​https://i.gyazo.com/cbcd703b8f149cbec9b335672568f1fc.gif
[Export][Button] Send Export Email - ​https://i.gyazo.com/770ba41ff9aa4a16ec2b3bd272cc3ddb.gif
[Erasure][Row Action] Force Erase Personal Data - ​https://i.gyazo.com/6058b038fd31b4c9bfb54d8352506353.gif
[Erasure][Button] Erase Personal Data - ​https://i.gyazo.com/fc9c9c7e57b069cc5f67717c92d75fb8.gif

[karmatosed]

One little note, is there any way we could as a v2 explore having a checkbox or link over multiple secondary buttons in the interface? Just an idea to avoid a sea of buttons in that column.

[xkon]

@karmatosed if I understand correctly you're talking about all the buttons on the "Next Steps" column, correct?

Those were originally designed as buttons to keep the spinners etc inside as a "group". They don't look really nice to me either and I would simply prefer normal links visually as well.

Before we change the scope of this ticket since we're close on commit, in general, let me see what we could do and get you a preview of how everything would look like as normal links ( some of them are actually already <a href> but they have the button class so they look like secondaries. ).

If you're ok with the preview we can open up a fresh ticket and just put a patch in to keep the scopes within limits and mark both for commit :-).

[karmatosed]

@xkon that sounds great, to be clear I would be ok exploring in a new ticket and merging this as is. I don't want to get in way of the flow here.

[xkon]

Sweet! I'll see what changes are needed as we might have to alter the JS part as well on some of those from a quick look that I've given it and I'll get a new ticket going asap directly with a patch :-). Thanks for the feedback!

[xkon]

Discussion from comment 12 regarding changing into links has been split here #49323 :).

[xkon]

Making a note here as well we'll need this first to go in so then we can refresh #49323 also as a complimentary patch.

[birgire]

Nice, we should also make sure the edge cases for zero count in:

var progress       = eraserIndex / erasersCount;

and

var progress       = eraserIndex / erasersCount;

will not give divison by zero.

I also wonder about the missing inline docs of the new JS functions.

[garrett-eclipse]

Refresh to avoid divisible by 0

[garrett-eclipse]

Thanks for the feedback @birgire I've refreshed to avoid division by zero in 44264.6.diff and opened #49381 to address the missing inline docs on the JS functions as there's none on any functions within the privacy-tools.js. Let's address that in a future release to avoid holding up this enhancement.

[birgire]

@garrett-eclipse thanks, sounds good

[SergeyBiryukov]

In 47246:

Privacy: Give progress indication for export and erasure.

This adds a progress indicator for "Download Personal Data" and "Erase Personal Data" row actions, which can take a while with a lot of data.

Props garrett-eclipse, allendav, dominic_ks, xkon, karmatosed, birgire.
Fixes #44264.

[SergeyBiryukov]

In 47248:

Coding Standards: Fix JSHint issues in [47246].

See #44264.

[pbiron]

Replying to garrett-eclipse:

I will note some minor issues with the UI which are addressed in other tickets.
1) If you're using the row actions and move your cursor away the action vanishes and along with it the progress there. This would be resolved if progress can be made on #48751.

So, I was playing with this in 5.4-beta1 and I really like it. I was going to propose making the progress indicator in the row action visible even if not hovered over...and then I see @garrett-eclipse already notes that as a possible deficiency.

I would be very much against the proposal in #48751 to make all rows actions always visible across all list tables.

I've got something working locally that makes the progress indicator visible (even if not hovered over) only while the download is happening. It needs a little more testing and when I've done that I'll add a patch for it.

[garrett-eclipse]

Reopening to accommodate making the indicators visible. Thanks @pbiron for looking into this.

[pbiron]

44264.7.diff keeps the row-action progress indicator visible while the export/erase is happening.

In keeping with the lack of JS Docs for the rest of the file, I didn't bother adding any for the 2 functions added by this patch :-) As mentioned in #comment:19, we can add those later.

But I'll explain how it works here:

1. in all list tables, row actions are wrapped in a div.row-actions
2. if div.row-actions also has the visible class, then there is CSS (in wp-admin/css/list-tables.css) that makes the row-actions visible even when they aren't hovered over
3. so, at the beginning of the export/erase click handlers, the patch simply adds the visible class
4. there is existing JS in wp-admin/js/common.js that makes row-actions visible/hidden when tabbing through the table. That JS only "does it's thing" for cells with the has-row-actions class
5. so, at the beginning of the export/erase click handlers, the patch also removes the has-row-actions class everywhere it appears in the list table
6. in the success/failure AJAX handlers, the patch removes the visible class from div.row-actions and adds the has-row-actions class back everywhere it was removed from

One consequence of step 5, is that if you tab through the table while the export/erase is happening, row-actions in other rows don't become visible. I think that's acceptable. Does anyone disagree?

[audrasjb]

Hi there,

With Beta 3 approaching, we'll need a decision and a commit action very soon.
For Component maintainers: if you don't feel the current patch is ready to land in WP 5.4 in the next few day, it's could be better to move it to 5.5. We're keeping it in the milestone for now.

[garrett-eclipse]

Alternate approach to keeping the row-actions for processing visible. Avoids breaking the tab functionality.

[garrett-eclipse]

Thanks for the patch @pbiron testing I found it worked but I was hesitant to go all in as I was able to produce the concern you had with breaking the tab action that makes row-actions visible.

In 44264.8.diff I took an alternate approach to avoid breaking that tab functionality. Instead of affecting the other classes to break the hover effect on the row actions I introduced a .processing class which matches the .visible class but won't be removed by other pre-existing functionality. This approach I found is less code heavy as well which is a bonus.

My approach works in that the .processing class is added when the action is initiated which keeps it in view while processing and upon completion or failure this class is removed so the normal show/hide on row-actions takes affect again. Meaning if you're hovering or tabbing the row actions work as normal.

While testing my approach one draw back initially was if you were hovered then 'Erasure completed' wouldn't become visible as that message is added after the processing. To account for this I wrapped the removeClass calls in SetTimeouts for half a second so the users is able to view/read the completion message before normal show/hide functionality takes over again.

Also note two other items in this patch;

1. The erasure click handler was missing the event.preventDefault(); so I added it to match with the export click handler.
2. There was a hide call on the handlers upon success that made it so additional exports weren't possible. This was added unintentionally when we tried to make the two processes match in [46412] / #43974. As this affects this process and is a reversion in and of itself I included in this patch. If needed can be split out to a new ticket/patch but feel it's fairly safe to fix two reversions in one go here.

@pbiron / @xkon / @SergeyBiryukov could you review/test and we can get this fixed hopefully before RC.

[pbiron]

Replying to garrett-eclipse:

In 44264.8.diff I took an alternate approach to avoid breaking that tab functionality. Instead of affecting the other classes to break the hover effect on the row actions I introduced a .processing class which matches the .visible class but won't be removed by other pre-existing functionality. This approach I found is less code heavy as well which is a bonus.

yes, less code is better :-) And yours seems to work great.

Also note two other items in this patch;

1. The erasure click handler was missing the event.preventDefault(); so I added it to match with the export click handler.
2. There was a hide call on the handlers upon success that made it so additional exports weren't possible. This was added unintentionally when we tried to make the two processes match in [46412] / #43974. As this affects this process and is a reversion in and of itself I included in this patch. If needed can be split out to a new ticket/patch but feel it's fairly safe to fix two reversions in one go here.

Re: number 2: I'd encountered that problem while testing the commit (before working on my patch), but couldn't track down what was causing it. I think fixing both of those is great.

I just noticed something else (I think that is unrelated to either your patch or mine)...give me a few more minutes to investigate it more.

[pbiron]

Replying to pbiron:

I just noticed something else (I think that is unrelated to either your patch or mine)...give me a few more minutes to investigate it more.

That one other thing is that upon successful export/erase, the span.{export,erasure}-progress value stayed at the percentage of the penultimate {export,eraser}Index.

44264.9.diff maintains everything from 44264.9.diff and ensures that the progress shows 100% upon success.

[garrett-eclipse]

Wrap the Success/Failure functions in SetTimeout for half a second so the user is able to see 100%

[garrett-eclipse]

Thanks @pbiron good catch on allowing it to get to 100% there. In my testing the view changed so fast after completion that I wasn't actually able to see it get to 100% so I updated the patch in 44264.10.diff in order to allow the user to see the 100% progress before it switches the view.

I feel that's a wrap and am pretty happy with things now, could you do a final review/test and we'll mark this for commit.

[pbiron]

Looks good to me. Marking for commit.

[SergeyBiryukov]

In 47395:

Privacy: Make the progress indicator for export and erasure visible even if not hovered over.

Follow-up to [47246].

Props pbiron, garrett-eclipse.
Fixes #44264.

[garrett-eclipse]

Dev Note: ​https://make.wordpress.org/core/2020/03/02/privacy-updates-in-5-4/