Fix/55 Fix anonymize functionality from CLI #77

bernattorras · 2023-01-25T15:52:41Z

Description of the issue

In #55 we decided to remove the anonymization functionality from the plugin, leaving only the CLI command to anonymize the users, orders, and customers.

When I tried the wp safety-net anonymize CLI command it got stuck and didn't anonymize any user data.

After taking a look at it, I've found that the problem was that the temporary tables weren't being used for copying, processing and storing the user details. These DB operations were only used for the background processes (which the CLI isn't using).

Changes proposed in this PR

Move these DB operations to new standalone functions
Call these functions when needed from the CLI command (before and after anonymizing the users)
Since the anonymization process affects the database and can cause some issues on big sites, I've added a warning and a prompt asking for the customer to confirm the execution of this functionality.
Added some feedback lines in the terminal to show the process of the command (it could help in case it gets stuck again due to any issues).

Steps to test

Use the trunk version of the plugin
Create some dummy users (i.e wp user generate 100)
Run the wp safety-net anonymize command
[It should get stuck without anonymizing any user]
Confirm that the users haven't been anonymized
Switch to this branch
Run the wp safety-net anonymize command again
Confirm that a warning appears and you're asked for a confirmation in order to continue with the anonymization.
Confirm that the anonymization process gets cancelled when choosing n.
Confirm that the anonymization process starts when choosing y.
Confirm that feedback lines appear before anonymizing users, orders and customers (check the previous image)
Confirm that the users/orders/customers data has been properly anonymized.

Notes

I've removed an orphan wp_ajax_safety_net_anonymize_users hook (not needed since this functionality was removed from the admin in Added actions toggle, removed anonymize #62.
I've updated the code of includes/amin.php to fix some phpcs issues (mostly related to shorthand syntax)
The wording of the warning or feedback lines that appear when running the CLI command can probably be improved. Feel free to review them and suggest any changes.

Fixes #55

tommusrhodus · 2023-01-27T19:23:12Z

@bernattorras I'm running into an issue running the testing steps at point 9. Output:

➜  public wp safety-net anonymize
This process will anonymize your current users, orders and customers with dummy data. 
Warning: Please proceed with caution if you have a site with a large number of users/orders/customers
Are you sure you want to do this? [y/n] y
- Copying users to temporary tables ...
- Anonymizing users ... 
<div id="error"><p class="wpdberror"><strong>WordPress database error:</strong> [Table &#039;local.wp_users_temp&#039; doesn&#039;t exist]<br /><code>SELECT ID 
				FROM wp_users_temp 
					LIMIT 500 
				OFFSET 0</code></p></div>

tommusrhodus

Great work! Left some comments on bits that jumped out at me.

tommusrhodus · 2023-01-27T19:34:57Z

includes/admin.php

@@ -45,14 +44,14 @@ function enqueue_scripts( string $hook_suffix ) {
 		return;
 	}

-	wp_enqueue_script( 'safety-net-admin', SAFETY_NET_URL . 'assets/js/safety-net-admin.js', [ 'jquery' ], '1.0', true );
+	wp_enqueue_script( 'safety-net-admin', SAFETY_NET_URL . 'assets/js/safety-net-admin.js', array( 'jquery' ), '1.0', true );


Could we tie asset version numbers to the plugin version, or maybe a constant defined in the main plugin file? Hard-coded version numbers may get forgotten.

I'm not 100% sure which would be the best way to version this script... Linking it to the plugin version seems too strict (as I understand that minor changes could be applied without updating the plugin version number).

I kinda feel the same way about generic asset versioning...

For now, I've decided to use the filemtime of the file. I know that this can generate false updates when using multiple servers though...

@tommusrhodus which approach would you recommend for this specific case?

tommusrhodus · 2023-01-27T20:01:57Z

includes/anonymize.php

+function store_anonymized_user_data() {
+	global $wpdb;
+
+	$wpdb->query( "INSERT INTO $wpdb->users (SELECT * FROM {$wpdb->users}_temp WHERE id NOT IN (SELECT ID FROM $wpdb->users))" );


Error handling needed for non-standard table that might not exist.

tommusrhodus · 2023-01-27T20:05:16Z

includes/cli/anonymize.php

+		WP_CLI::confirm( 'Are you sure you want to do this?' );
+
+		WP_CLI::log( '- Copying users to temporary tables ...' );
+		copy_and_clear_user_tables();


Do any of these functions return data that could be shown to the user? e.g error handling, success message, count of changes rows etc?

tommusrhodus · 2023-01-27T20:08:55Z

includes/admin.php

 	} ?>

-	<?php if ( 'checkbox' === $args['type'] ) : 
+	<?php


Closing and opening PHP tags aren't needed here.

^ nitpicking 🤣

tommusrhodus · 2023-01-27T20:12:00Z

includes/cli/anonymize.php

 		anonymize_users();

+		WP_CLI::log( '- Storing the anonymized users to the default tables ... ' );
+		store_anonymized_user_data();


Each of these steps definitely needs some more verbosity. A nice success message if everything goes to plan, and a detailed error output if not ideally.

bernattorras added 5 commits January 25, 2023 16:22

Remove 'wp_ajax_safety_net_anonymize_users' hook & fix some phpcs

6c6d2b9

Moved DB operations into new functions

3cc90d1

Moved DB operations to store_anonymized_user_data fnc

573a44d

Create and delete the tmp tables before anonymizing

5a36139

Add feedback lines and ask for confirmation

da906c0

bernattorras requested a review from NickGreen January 25, 2023 15:52

bernattorras self-assigned this Jan 25, 2023

bernattorras mentioned this pull request Jan 25, 2023

Remove Anonymization Functionality from plugin #55

Closed

tommusrhodus reviewed Jan 27, 2023

View reviewed changes

Added script versioning (filetime)

349e9a7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/55 Fix anonymize functionality from CLI #77

Fix/55 Fix anonymize functionality from CLI #77

bernattorras commented Jan 25, 2023

tommusrhodus commented Jan 27, 2023

tommusrhodus left a comment

tommusrhodus Jan 27, 2023

bernattorras Mar 7, 2023

tommusrhodus Jan 27, 2023

tommusrhodus Jan 27, 2023

tommusrhodus Jan 27, 2023

tommusrhodus Jan 27, 2023

tommusrhodus Jan 27, 2023

Fix/55 Fix anonymize functionality from CLI #77

Are you sure you want to change the base?

Fix/55 Fix anonymize functionality from CLI #77

Conversation

bernattorras commented Jan 25, 2023

Description of the issue

Changes proposed in this PR

Steps to test

Notes

tommusrhodus commented Jan 27, 2023

tommusrhodus left a comment

Choose a reason for hiding this comment

tommusrhodus Jan 27, 2023

Choose a reason for hiding this comment

bernattorras Mar 7, 2023

Choose a reason for hiding this comment

tommusrhodus Jan 27, 2023

Choose a reason for hiding this comment

tommusrhodus Jan 27, 2023

Choose a reason for hiding this comment

tommusrhodus Jan 27, 2023

Choose a reason for hiding this comment

tommusrhodus Jan 27, 2023

Choose a reason for hiding this comment

tommusrhodus Jan 27, 2023

Choose a reason for hiding this comment