Skip to content
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

Support for api/v1/accounts/update_credentials route #788

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

mattwiebe
Copy link
Contributor

Running this along with akirk/enable-mastodon-apps#157 ~= profile editing support via 3rd party Mastodon apps :D

Proposed changes:

Other information:

  • Have you written new tests for your changes, if applicable?

Testing instructions:

  • Go to '..'

@pfefferle
Copy link
Member

pfefferle commented Jun 24, 2024

I'm not sure if it's a good idea to store the information directly in the setter!? I think that's not what a model/bean is meant to be!?

@mattwiebe
Copy link
Contributor Author

I'm not sure if it's a good idea to store the information directly in the setter!? I think that's not what a model/bean is meant to be!?

Can you explain more? I don't follow. I've used Models to coordinate CRUD layers before, but always open to learning a better way of doing things!

@pfefferle
Copy link
Member

pfefferle commented Jun 25, 2024

I am sorry, my brain is still a but fuzzy!

It is not about the CRUD idea, it is about directly saving the data to the DB. Normally getters and setters are to get and set object attributes. I know that we are not consistent yet, because the getters also directly load stuff from the DB (maybe we have to refactor this too), but the from_array and from_json methods do use the set_ methods to prefill the objects and this might cause strange side effects if we write directly to the db.

Maybe we should use this feature to refactor the code a bit and to consistently use attributes and maybe have a save function, that finally persists things.

$icon_id = (int) $data['avatar'];
$attachment = \get_post( $icon_id );
if ( $attachment && 'attachment' === $attachment->post_type ) {
l( 'set_icon' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should cause errors outside of the wp.com context.

@mattwiebe mattwiebe force-pushed the add/enable-mastodon-apps-profile-editing branch from fcd97be to 29db809 Compare June 25, 2024 21:18
*
* @return int The user id
*/
public static function maybe_map_user_to_blog( $user_id ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this working?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is! I use it inside a couple of our callbacks to change somebody's WP User ID to the Blog User ID only when the user type is disabled and the blog is enabled. In this way a user who is logged in is doing what the plugin supports: only the Blog User. It does not allow us to log in as the Blog User when both Blog and User types are enabled, but that seems to be a much smaller problem overall.

Screenshot 2024-06-26 at 12 48 08

* @param string $name The User-Name.
* @return bool True if the User-Name was updated, false otherwise.
*/
public function save_name( $name ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to move the save functions to the EMA-API class instead then, because they are very specific and I do not know if we can re-use them in other parts of the code.

Copy link
Contributor Author

@mattwiebe mattwiebe Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the User type I completely agree, as long as we and EMA are both using the same attributes

EDIT Although one problem there is how to store avatars and headers

@pfefferle
Copy link
Member

@mattwiebe There is no way you can login as the blog author (yet), or do you have found a way?

The other thing: Some of the informations seem to be generic and not related to ActivityPub, so why not move them to Enable-Mastodon-Apps instead? Like save_name and save_description for example.

$icon = wp_get_attachment_url( $maybe_id );
}

$option_name = 'activitypub_user_icon_' . $this->_id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For signatures we use $user->user_login instead, because the ID can change on migrations!?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a pretty good idea. For dotcom Simple -> Atomic migrations we do maintain the User ID although that may not always be the case when we do something like a WXR Import

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we should be consistent! I am already having a hard time with this login key on the deletion feature. If we decide to use the id instead then I will migrate that stuff!

@mattwiebe mattwiebe force-pushed the add/enable-mastodon-apps-profile-editing branch from bbe2a01 to a33cbd2 Compare June 26, 2024 02:19
@mattwiebe
Copy link
Contributor Author

@mattwiebe There is no way you can login as the blog author (yet), or do you have found a way?

Yup there is! See above, only when only the Blog User is active.

The other thing: Some of the informations seem to be generic and not related to ActivityPub, so why not move them to Enable-Mastodon-Apps instead? Like save_name and save_description for example.

Those easily could, and should be supported in EMA itself, but it doesn't yet have a concept of header or avatar. The latter is still managed in Core through Gravatar, and there is only the (deprecated in Block Themes) header_image for the whole site. So perhaps EMA supports name and description, but we do avatar, header, and soon, extra fields

public function save( $key, $value ) {
switch ( $key ) {
case 'name':
return \update_option( 'blogname', $value );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems a bit dangerous, you might not realize that you're changing the whole blog name. Should this just be an override?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's tricky. Once we introduce an override, now you have to go into wp-admin to also change the blogname, if that's what you want to do, and/or being able to delete the override, falling back to the blogname, which you can now only edit in wp-admin. If you want a consistent name, you have to go to wp-admin. I'm thinking more of somebody who never wants to go there at all.

I'm erring on the side of consistency, not on the side of safety. And if somebody doesn't like the change they made, they can go back and change it again, without having to go to wp-admin.

At least that's the kind of thinking I was doing when I made it behave this way

case 'name':
return \update_option( 'blogname', $value );
case 'summary':
return \update_option( 'blogdescription', $value );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, maybe rather with an override?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants