80 wp user avatar

[claytoncollie]

Provides admin-ajax and wp-cli methods for migrating avatars away from the WP User Avatar plugin.

Addresses #80

Verification Process

Visit /wp-admin/options-discussion.php to find the button at the bottom of the page to use the admin-ajax version.

Or run simple-local-avatars migrate wp-user-avatar to use the wp-cli version.

In both cases, we create a database option simple_local_avatars_migrations to store previously migrated avatars.

Checklist:

• I have read the CONTRIBUTING document.
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my change.
• All new and existing tests passed.

Changelog Entry

• Add wp-cli command to migrate avatars from WP User Avatars
• Add dashboard setting to migrate avatars from WP User Avatars

[claytoncollie]

@jeffpaul I had to get this over to you to at least start a conversation or else it would drag on too much. I know there is a lot of code duplication between the ajax version and the cli version. The CLI warning, error, and success messages sprinkled throughout the method made it a bit more difficult to re-use for the ajax version. I'm happy to DRY it out but wanted to at least get your eyes on it before wasting much more time. Happy to make any and all revisions you see fit.

[helen]

Overall looks reasonable code-wise, I did not functionally test it for the moment. Left inline notes about smaller things, as for the DRY part I do think it's worth doing for consistency and sanity. You should be able to split the code paths within one method with the WP_CLI constant, and the AJAX version really should return a response status and message to show user feedback on the screen, e.g. "successfully migrated X avatars" or a descriptive error message.

[claytoncollie]

@helen Thanks for the feedback. I refactored the migration method to return an integer based on how many avatars were processed. That method is called in the ajax or the wp-cli methods then the value is used in the messaging. I still have some work to do with javascript error messaging and spinner for the dashboard version.

[claytoncollie]

@helen I added localized success and error messages to the admin version so the user can see how many avatars were processed. I am very close to finishing my work depending on your feedback. This could use another code review when you have a minute.

[claytoncollie]

@faisal-alvi Please unapprove this PR. I have not fixed the major issue of it not working on single-site installs. get_sites is only available for multisite installs so I need to refactor a bit.

[claytoncollie]

@faisal-alvi This is ready for review. I added support for single-site installs.

Testing Steps

1. Install WP user Avatar
2. Create a new user and give them an avatar image in the WP User Avatars field.
3. Run the WP CLI command or use the ajax button in the admin to migrate it.
   wp simple-local-avatars migrate wp-user-avatar --yes
   or
   wp simple-local-avatars migrate wp-user-avatar
4. Check the database table *_usermeta for simple_local_avatar to see if the image migrated properly.
5. Check the database table *_options for simple_local_avatars_migrations to see if the user ID was saved.

Repeat steps for single site install and multisite install.

[faisal-alvi]

LGTM! Just a question asked, good to be merged.

[claytoncollie]

@faisal-alvi I am not sure what the release process is for this plugin BUT all of the JS from this PR lives in the .dev file so that probably needs to be compiled into the production simple-loca-avatars.js file. I also have not updated the readme.

[faisal-alvi]

@claytoncollie I have re-checked the functionality. When trying to follow these steps, on step no 2, I can not select any image for the avatar. It works for the develop branch. Please have a look.

sla-avtar-issue-pr85.mp4

[faisal-alvi]

Re-requesting changes to fix the avatar selection issue.

[faisal-alvi]

@claytoncollie regarding your JS question, we use uglyjs to minify it only, please use it and can you please also add the npm command to do it?

[claytoncollie]

@faisal-alvi I reverted how we are grabbing the user_id on the edit-user.php screen. That was breaking the javascript on that screen.

simple-local-avatars/includes/class-simple-local-avatars.php

Line 284 in 9499974

$user_id = ( 'profile.php' === $hook_suffix ) ? get_current_user_id() : (int) $_GET['user_id'];

(int) $_GET('user_id') works fine. I changed that to get_query_var('user_id') which is what broke it. This is not code that I originally wrote and was instructed to alter to not have $_GET in the code. I think we should leave this as is. It is not related to this PR.

As for the minified JS, I do not feel comfortable adding that to this PR. There is no package.json file in this plugin. Can you all take care of minification in the release branch in case there are other changes from other PRs?

[faisal-alvi]

Thanks for the changes @claytoncollie.

1. Let's leave the (int) $_GET('user_id') which is not related to the current PR.
2. I have added package.json and also added uglifyjs (in Introducing Package file #94) so we just have to run npm run uglifyjs and it will minify the dev JS file and update the content of the simple-local-avatars.js file (which is a minfied version of the dev file).

Leaving this PR until #94 is merged, then you can run the command and we are good to merge.

[faisal-alvi]

@jeffpaul we should add minifying the JS file as a checklist item in our release process. After #94 merge, we can use npm run uglifyjs command to minify JS at the time of release.

[jeffpaul]

@faisal-alvi seems like maybe we could add that to https://github.com/10up/simple-local-avatars/blob/develop/.github/workflows/push-deploy.yml and potentially remove the minified JS file from the repo as that'll get generated as part of the WP.org deploy action?

[faisal-alvi]

@jeffpaul that's great! done => 2064ba0

[faisal-alvi]

@claytoncollie I have merged develop to the 80-wp-user-avatar branch to resolve the conflicts. The PR is working fine. However, I have noticed a couple of things while testing.

1. The migration works only once. After the first migration, if a user re-uses the image from the "WP User Avatar" field and re-try to migrate, it does not work.
2. When a user is using an avatar from the "WP User Avatar" plugin, then if he tries to upload an image in the SLA avatar field, a new image does not reflect. When we deactivate the "WP User Avatar" plugin or remove the "WP User Avatar" image, the SLA avatar starts displaying. It seems like "WP User Avatar" has higher priority over SLA avatars.

@jeffpaul can you please let me know if these are intentional behaviors or should be considered bugs?

[jeffpaul]

1. Not sure I understand this flow, could you please restate?
2. That seems like something that should be changed, if someone attempts to change the image via the standard SLA flow then that image should take priority over whatever was migrated in from WP User Avatar.

[faisal-alvi]

Point no 1:

The migration works only once. After the first migration, if a user re-uses the image from the "WP User Avatar" field and re-try to migrate, it does not work.

• For example, a site admin is using "WP User Avatar" and the site has 100 users with avatars.
• Admin wants to move to SLA.
• SLA installed & activated.
• Admin visits Settings > Discussion and migrates avatars to SLA.
  
• But admin still uses "WP User Avatar" for 10 new users and 5 existing users.
• After some time, the admin wants to re-migrate and visits Settings > Discussion and clicks migrate button.
• Expectation would be: 10 new + 5 existing user avatars should be migrated.
• Reality is: only 10 new user avatars migrated.
• SLA does not re-migrate 5 exiting users as they are already "marked as migrated" somewhere in DB.
• Is this behavior expected or a bug?

Point no 2:

When a user is using an avatar from the "WP User Avatar" plugin, then if he tries to upload an image in the SLA avatar field, a new image does not reflect. When we deactivate the "WP User Avatar" plugin or remove the "WP User Avatar" image, the SLA avatar starts displaying. It seems like "WP User Avatar" has higher priority over SLA avatars.

• For example, both plugins (WP User Avatar & SLA) are installed and active on the site.
• Admin sets an avatar using the "WP User Avatar" field for a user.
  
• After clicking "update", the SLA field also displays the same avatar set in the "WP User Avatar" field.
• After some time, the admin sets a different avatar, but this time using the "SLA" avatar field.
• The different avatar set via SLA does not display. An old avatar from "WP User Avatar" still displays (seems due to a higher priority).
• If admin removes avatar from "WP User Avatar" field OR deactivates "WP User Avatar" plugin, the new SLA avatar starts displaying!
• Is this behavior expected or a bug?

[claytoncollie]

@faisal-alvi Part 1 is intended functionality. That is the entire reason behind storing the IDs in the options table. Our thinking was that if the site has a large number of users to migrate, the admin-ajax function might timeout before completion. And in that case, if it times out halfway through we have a record and can start over at the end of the record instead of from the very beginning. This might have been spoken about in this thread or on a zoom with @helen

Part 2 seems to be a "feature" of WP User Avatar. That does make sense to me that a plugin name starting with the letter W is loading after a plugin name starting with S thus firing all of its filter and actions after SLA. I think if people have both plugins installed, the intended outcome is that they migrate and then deactivate the WPUA plugin. Am I giving the average user too much credit? One thing this plugin does not do is delete or modify any WPUA data. That might be a solution but we felt it was not our responsibility.

[faisal-alvi]

@claytoncollie thanks for the answers, @jeffpaul we are good to go 🚀