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

Enable Mastodon Apps: support profile editing, blog user #788

Merged
merged 42 commits into from
Sep 23, 2024

Conversation

mattwiebe
Copy link
Contributor

@mattwiebe mattwiebe commented Jun 23, 2024

Below is a screen recording of me using this PR to drive a Mastodon app, providing full profile editing support through 3rd party apps. (still requires akirk/enable-mastodon-apps#167 to work for Extra Fields)

Screen.Recording.2024-09-16.at.15.34.46.mov

Proposed changes:

  • Fully support the profile editing route: name, bio, avatar, header, extra fields
  • Use the Blog User account in the app when only the blog user is enabled

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.

@mattwiebe mattwiebe force-pushed the add/enable-mastodon-apps-profile-editing branch from fcd97be to 29db809 Compare June 25, 2024 21:18
@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.

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

* - header: The User-Header-Image.
* @return bool True if the attribute was updated, false otherwise.
*/
public function save( $key, $value ) {
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 rename that, so that we will be able to have a generic save in the future, that saves all attributes to the DB.

Copy link
Member

Choose a reason for hiding this comment

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

maybe something like save_attribute or so?!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to have dedicated functions. I went in an update_key direction with it.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for maybe not being precise enough: My proposal was to simply not use the generic save function, but something different instead. I am fine with having one function with key => value params, but I am also fine with dedicated functions :)

@mattwiebe
Copy link
Contributor Author

Now that akirk/enable-mastodon-apps#157 is merged, my plan is to first get #838 merged, and then rework this PR to fully take advantage of 1) Header Images and 2) Extra Fields. So close!

Only Extra Fields remains of that. We could probably just ship Extra Fields in a subsequent PR, but otherwise I'll wrap this up early next week.

@mattwiebe
Copy link
Contributor Author

Extra Fields support went quicker than expected!

We will require akirk/enable-mastodon-apps#167 to test. I will come back and try to form some testing instructions later.

return '<!-- wp:paragraph --><p>' . $content . '</p><!-- /wp:paragraph -->';
}

public static function set_extra_fields_from_mastodon_api( $user_id, $fields ) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have all "enable mastodon apps" functions in the integration file and not mix them up with the "core" files!?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup sounds good to me

@mattwiebe mattwiebe changed the title Support for api/v1/accounts/update_credentials route Enable Mastodon Apps: support profile editing, blog user Sep 17, 2024
<?php
esc_html_e( 'These are extra fields that are used for your ActivityPub profile. You can use your homepage, social profiles, pronouns, age, anything you want.', 'activitypub' );
echo '<br />';
esc_html_e( 'Note that Mastodon clients will only show four fields.', 'activitypub' );
Copy link
Member

@pfefferle pfefferle Sep 20, 2024

Choose a reason for hiding this comment

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

That is not completely correct! You can only add 4 extra fields through the Mastodon frontend, but it displays way more than these 4 when a different platform supports/provides it.

See my blog as example:

Screenshot 2024-09-20 at 09 40 50 Screenshot 2024-09-20 at 09 44 26

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice find, I foolishly believed something I read in an API doc rather than testing things :D

@mattwiebe mattwiebe force-pushed the add/enable-mastodon-apps-profile-editing branch from b7269ec to d022392 Compare September 20, 2024 15:30
@mattwiebe mattwiebe merged commit 9929571 into master Sep 23, 2024
21 checks passed
@mattwiebe mattwiebe deleted the add/enable-mastodon-apps-profile-editing branch September 23, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants