WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 4 weeks ago

Last modified 4 weeks ago

#44264 closed enhancement (fixed)

Give progress indication for export and erasure

Reported by: allendav Owned by: garrett-eclipse
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.9.6
Component: Privacy Keywords: has-patch has-screenshots needs-testing commit has-dev-note
Focuses: ui, javascript Cc:

Description

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.

Attachments (15)

44264.0 (4.5 KB) - added by dominic_ks 6 months ago.
Initial patch to show percentage completion of export or deletion.
44264.1.patch (5.1 KB) - added by dominic_ks 6 months ago.
Updated patch with fixed indents and also allows for specific progress indicators per job when running multiple exports or deletions at the same time.
44264.2.diff (5.2 KB) - added by garrett-eclipse 3 months ago.
Refreshed patch to address PHPCS issues and some naming changes
cb02c181f77525a819219c55ea6a7711.gif (356.1 KB) - added by garrett-eclipse 3 months ago.
Send Export Link w/ Progress
0a0b21446f018d6f2a0a87c47daffe63.gif (377.4 KB) - added by garrett-eclipse 3 months ago.
Erase Personal Data w/ Progress
Screen Shot 2019-12-20 at 10.42.46 AM.png (25.9 KB) - added by garrett-eclipse 3 months ago.
The 'Download Personal Data' row-action which potentially could have the progress indicator
Screen Shot 2019-12-20 at 10.42.53 AM.png (17.5 KB) - added by garrett-eclipse 3 months ago.
The 'Force Erase Personal Data' row-action which potentially could have the progress indicator
44264.3.diff (5.1 KB) - added by garrett-eclipse 3 months ago.
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
44264.4.diff (7.4 KB) - added by xkon 3 months ago.
44264.5.diff (6.7 KB) - added by garrett-eclipse 2 months ago.
Minor refresh to remove unneeded extra concat for space before span.
44264.6.diff (6.7 KB) - added by garrett-eclipse 7 weeks ago.
Refresh to avoid divisible by 0
44264.7.diff (4.4 KB) - added by pbiron 6 weeks ago.
44264.8.diff (3.1 KB) - added by garrett-eclipse 4 weeks ago.
Alternate approach to keeping the row-actions for processing visible. Avoids breaking the tab functionality.
44264.9.diff (4.6 KB) - added by pbiron 4 weeks ago.
44264.10.diff (4.8 KB) - added by garrett-eclipse 4 weeks ago.
Wrap the Success/Failure functions in SetTimeout for half a second so the user is able to see 100%

Download all attachments as: .zip

Change History (53)

#1 @desrosj
21 months ago

  • Keywords gdpr removed

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

#2 @garrett-eclipse
6 months ago

  • Focuses ui javascript added
  • Keywords good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

This ticket was mentioned in Slack in #core by dominic_ks. View the logs.


6 months ago

#4 @dominic_ks
6 months ago

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
6 months ago

Initial patch to show percentage completion of export or deletion.

#5 @dominic_ks
6 months ago

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.

#6 @rukkykf
6 months ago

  • Keywords has-patch added; needs-patch removed

#7 @rukkykf
6 months ago

  • Keywords needs-testing added

#8 @garrett-eclipse
6 months ago

  • Keywords needs-refresh added; good-first-bug needs-testing removed

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
6 months ago

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
3 months ago

Refreshed patch to address PHPCS issues and some naming changes

@garrett-eclipse
3 months ago

Send Export Link w/ Progress

@garrett-eclipse
3 months ago

Erase Personal Data w/ Progress

#9 @garrett-eclipse
3 months ago

  • Keywords has-screenshots needs-testing needs-design-feedback added; needs-design needs-refresh removed
  • Milestone changed from Future Release to 5.4
  • Owner set to garrett-eclipse
  • Status changed from new to accepted

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
3 months ago

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

@garrett-eclipse
3 months ago

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

@garrett-eclipse
3 months ago

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
3 months ago

#10 @xkon
3 months ago

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
2 months ago

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

#11 follow-up: @garrett-eclipse
2 months ago

  • Keywords commit added; needs-testing needs-design-feedback removed

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

#12 @karmatosed
2 months ago

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.

#13 @xkon
2 months ago

@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 :-).

Last edited 2 months ago by xkon (previous) (diff)

#14 @karmatosed
2 months ago

@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.

#15 @xkon
2 months ago

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!

#16 @xkon
2 months ago

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

#17 @xkon
8 weeks ago

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.

#18 @birgire
7 weeks ago

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
7 weeks ago

Refresh to avoid divisible by 0

#19 @garrett-eclipse
7 weeks ago

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.

#20 @birgire
7 weeks ago

@garrett-eclipse thanks, sounds good

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


7 weeks ago

#22 @SergeyBiryukov
7 weeks ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#23 @SergeyBiryukov
7 weeks ago

In 47248:

Coding Standards: Fix JSHint issues in [47246].

See #44264.

This ticket was mentioned in Slack in #core-privacy by xkon. View the logs.


6 weeks ago

#25 in reply to: ↑ 11 @pbiron
6 weeks ago

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.

#26 @garrett-eclipse
6 weeks ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

@pbiron
6 weeks ago

#27 @pbiron
6 weeks ago

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?

This ticket was mentioned in Slack in #core-privacy by pbiron. View the logs.


6 weeks ago

#29 @audrasjb
5 weeks ago

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.

This ticket was mentioned in Slack in #core-privacy by pbiron. View the logs.


5 weeks ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 weeks ago

@garrett-eclipse
4 weeks ago

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

#32 follow-up: @garrett-eclipse
4 weeks ago

  • Keywords needs-testing added

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.

#33 in reply to: ↑ 32 ; follow-up: @pbiron
4 weeks ago

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
4 weeks ago

#34 in reply to: ↑ 33 @pbiron
4 weeks ago

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
4 weeks ago

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

#35 @garrett-eclipse
4 weeks ago

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.

#36 @pbiron
4 weeks ago

  • Keywords commit added

Looks good to me. Marking for commit.

#37 @SergeyBiryukov
4 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.