-
Notifications
You must be signed in to change notification settings - Fork 202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow guest authors have personal data exported #528
Conversation
If something change, we can't forget change too: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @mariovalney; this looks great.
I've flagged a few issues and left some suggestions we should fix before merging.
} | ||
|
||
$author_data = array( | ||
'ID' => __( 'Guest Author ID', 'co-authors-plus' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove Guest Author
from the labels here as it's a bit redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it because user export has "User" label as well. But I agree it's redundant.
$author_data_to_export = array(); | ||
|
||
foreach ( $author_data as $key => $name ) { | ||
if ( empty( $author->$key ) ) continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: add braces for the if
and move continue
to new line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* @param int $author->ID The guest author ID | ||
* @param string $email_address The guest author email address | ||
*/ | ||
$extra_data = apply_filters( 'coauthors_guest_authors_exported_extra_data', [], $author->ID, $email_address ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a separate array that we have to merge in, maybe we should just pass $author_data_to_export
and other plugins can append to it directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, for consistency, we should call this coauthors_guest_author_personal_export_data
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filter changed but I think we should keep the merge. This way we don't allow other plugins to remove our data, which I think is mandatory to keep because GDPR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense @mariovalney. I think because of we should rename the filter to coauthors_guest_author_personal_export_extra_data
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
); | ||
|
||
return $exporters; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: indentation looks off here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thank you @mjangda ! I think it's ok now. |
foreach ( $author_data as $key => $name ) { | ||
if ( empty( $author->$key ) ) { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the indentation is off here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sublime is playing with me hahaha
I guess we should consider to add a .editoconfig
Thank you again @mariovalney! I just noticed two small things. We can merge after those are fixed. |
* [Style] Identation changed to tab * [Feat] Added personal data exporter to guest authors * [Feat] Added a filter to allow third part plugins add guest author data * [Style] WPCS fixes * [Fix] Filter name changed * [Style] WPCS fixes * [Fix] Filter name changed
This add the enhancement #518 to allow guest authors export their data as a regular author.
A new filters was introduced: coauthors_guest_authors_exported_extra_data.