Issue/12697 fix default blog

[aerych]

Fixes #12697

This is one possible approach to resolving #12697. I had it mostly written before I started second guessing whether it would just be more straightforward to fetch account details a second time during auth. It seems a little wasteful, but this patch seems a little heavy handed. 🤷‍♂ Maybe there is another option? Maybe syncing details is actually the way to go? Open to other ideas.

This PR bumps the model revision to 91 and adds a new field to WPAccount: primaryBlogID which is set when account details are synced. When an account is saved, it attempts to set its defaultBlog relation if one doesn't already exist. Its a small amount of overhead on every account save. When syncing account details, or when syncing blogs the default blog is updated, and the app extensions are configured.

To test:

• Start with a fresh install.
• Log into the app.
• As soon as you're logged in background the app and attempt to use the share extension via Safari.
• Confirm you don't receive an error.

For a more granular test:

• Start with a fresh install.
• Set a break point in WPAccount setupAppExtensionsWithDefaultAccount.
• Log in and confirm this method is called during the login flow.
• Visit the Me tab (which syncs account details), and confirm the method is called.
• Visit the blog list (which syncs blogs) and confirm this method is called.

cc @leandroalonso in case we decide to land this PR, cos you have another migration in the works.

Update release notes:

• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile]

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

[aerych]

I noticed that unit tests were failing and while looking into the cause realized I'd overlooked ensuring that the method that configures app extensions was called. 🤦‍♂ Updated, and ready for some early feedback.

[danielebogo]

Thanks @aerych for taking care of this!

[frosty]

Thanks for working on this!

So if I'm completely honest, I don’t know which is the best solution for this bug 🤔

I tested this PR and it works as expected and the code looks good 👍

But having a stored property for the blog ID feels like… a workaround? (which I guess it is!) We have two properties for the same thing, and one only exists so we can set up the other.

What are your thoughts about fetching user details twice? Could we use the fields property and specify just primary_blog to reduce the size of the second response? I don't know if this is any better than the primary blog ID property 🤷🏻‍♂️

[frosty]

Ideally I guess this should happen whenever primaryBlogID is modified from anywhere. I think you could use managedObjectOriginal_ to override the setter – something like this:

- (void)setPrimaryBlogID:(NSNumber *)value
{
     [self managedObjectOriginal_setPrimaryBlogID:value];
     
    // update the default blog
}

But not a necessity, just an idea! (reference for this is at the bottom of this page: https://developer.apple.com/library/archive/releasenotes/General/WhatNewCoreData2016/ReleaseNotes.html#//apple_ref/doc/uid/TP40017342-CH1-DontLinkElementID_10)

[aerych]

TIL! Thanks for that tip. My first take on this was to rely on willSave to ensure the the defaultBlog was updated. A bit overkill to do it that way. This would be much nicer. :)

[aerych]

Side note... It doesn't seem proper for a model to call out to a service to work, which is partly why I ditched the willSave approach--the service seemed best suited for updating the extensions. That would be my push-back on doing the work in the model's setter.
Another point if favor of nuking the relation? 😁

[frosty]

Yeah, I think I'd agree with that – maybe it doesn't make sense for the model to do this.

[aerych]

But having a stored property for the blog ID feels like… a workaround? (which I guess it is!) We have two properties for the same thing, and one only exists so we can set up the other.

It totally is a work around. I'd prefer to store the primaryBlogID given that it's real data provided by the api, and get rid of the defaultBlog relation. We could make defaultBlog a convenience accessor that lazy fetches the blog for primaryBlogID. I stopped short of this because it's a much more impactful change--defaultBlog is used in a lot of places!

What are your thoughts about fetching user details twice? Could we use the fields property and specify just primary_blog to reduce the size of the second response? I don't know if this is any better than the primary blog ID property 🤷🏻‍♂️

It seems wasteful to request the data a second time. That said, we could be sneaky sneaky and maybe hang on to the remote user response from the first call and "fake" the second call after blogs are synced. It's a total hack (more so than this PR already is 😬) but would be another option. Mabye. I haven't looked into it yet.

As far as a quick fix goes... it's really a coin toss for me to do something like this PR or to fetch a second time. Either way I think the best solution is what I outlined above... store the primaryBlogID and refactor the relation to a lazy getter. I'd be happy to write that patch but it might be a bit before I can make the time 😬.

[frosty]

I'd prefer to store the primaryBlogID given that it's real data provided by the api, and get rid of the defaultBlog relation.
store the primaryBlogID and refactor the relation to a lazy getter. I'd be happy to write that patch but it might be a bit before I can make the time 😬.

How about then we merge this PR to fix the original issue, and we can look at moving to that solution in a future patch? I agree that faking a second call is even more hacky 😅

[aerych]

How about then we merge this PR to fix the original issue, and we can look at moving to that solution in a future patch?

Sounds good. Marking this one ready for review. If you'll bestow a I'll merge it up.

[frosty]

!

[aerych]

Thanks @frosty !