Add Post Settings to Custom Posts#25268
Conversation
Generated by 🚫 Danger |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 31057 | |
| Version | PR #25268 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 6c7db82 | |
| Installation URL | 2eg9en7u2k92o |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 31057 | |
| Version | PR #25268 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 6c7db82 | |
| Installation URL | 3ulo45fg5vjmg |
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
| @Test("Post capabilities match expected values") | ||
| func testPostCapabilities() { | ||
| let caps = PostSettingsCapabilities.post() | ||
| #expect(caps.supportsCategories == true) |
There was a problem hiding this comment.
I'd suggest removing these tests as they duplicate the implementation:
/// Capabilities for the built-in "post" type.
static func post() -> PostSettingsCapabilities {
PostSettingsCapabilities(
supportsCategories: true,
supportsTags: true,
supportsFeaturedImage: true,
supportsExcerpt: true,
supportsAuthor: true,
supportsPostFormats: true,
supportsComments: true,
supportsTrackbacks: true,
supportsPageAttributes: false,
supportsSlug: true,
supportsCustomFields: true,
customTaxonomySlugs: []
)
}
|
|
||
| // Categories and tags are not yet derived from taxonomies (see FIXME in implementation). | ||
| #expect(caps.supportsCategories == false) | ||
| #expect(caps.supportsTags == false) |
There was a problem hiding this comment.
(nit) simple boolean expectations can be written as :
#expect(caps.supportsTags)
#expect(!caps.supportsTags)| case .customPostTypes: | ||
| return BuildConfiguration.current == .debug | ||
| case .cptPostSettings: | ||
| return false |
There was a problem hiding this comment.
Shouldn't it be enabled for the .debug configuration like other CPT-related flags?
There was a problem hiding this comment.
Doesn't syncBlogAndAllMetadata now cover refreshing custom types #25267?
There was a problem hiding this comment.
I also noticed new warnings in BlogService:
| // Refresh post in to keep the post list up-to-date | ||
| do { | ||
| try await client.service.posts().refreshPost( | ||
| postId: updatedPost.id, endpointType: endpoint |
There was a problem hiding this comment.
(outside of the scope of PR) The fact that you need to manually refresh the cache seems like a bit of a design issue here.
| static func == (lhs: Author, rhs: Author) -> Bool { | ||
| // The displayName may be fetched locally. | ||
| // Only id is sent to the API for updating author. | ||
| lhs.id == rhs.id |
There was a problem hiding this comment.
I'd consider using Identifiable instead of Equatable for whatever scenario it is used for or excluding fields like displayName and avatarURL from PostSettings – it seems they belong elsewhere. It's additional information required for rendering the form. It's not required for post settings themselves.
There was a problem hiding this comment.
The customized Equatable implementation is for the hasChanges = getSettingsToSave(for: new) != originalSettings statement. The originalSettings does not have displayName, because the name is not returned in the core REST API. The view model has code to update the Author instance with a displayName, so hasChanges would become true without this custom Equtable implementation, which is a bug because the author is not changed.
Considering the type Author here is internal to the Post Settings feature, I thought it'd be okay to change the Equtable implementation, instead of using the sythensized one.
There was a problem hiding this comment.
This seems like an acceptable workaround, but I think I made a design mistake when I added displayName and avatarURL to PostSettings. These fields are not required to update the author – you only need an ID. How these authors are displayed in the UI are a separate concern.
I'm going to leave it up to you since it's not as straightforward to change the original design now.
| otherTerms = [:] | ||
|
|
||
| // FIXME: Post metadata is not supported yet. Require wordpress-rs changes. | ||
| metadata = PostMetadata(from: .init()) |
There was a problem hiding this comment.
Yeah, metadata is not going to be easy.
|
|
||
| /// Creates `PostUpdateParams` representing the diff between the post and | ||
| /// the current settings, for use with the WordPress REST API. | ||
| func makeUpdateParameters(from post: AnyPostWithEditContext) -> PostUpdateParams { |
There was a problem hiding this comment.
(just noting) This looks similar to how diffing works with WordPressKit and Posts. Unfortunately, we now have quite a lot of duplication, but there doesn't seem to be way around that.
| } | ||
| featuredImageSection | ||
| if viewModel.isPost { | ||
| if viewModel.capabilities.supportsFeaturedImage { |
There was a problem hiding this comment.
(nit) It probably wouldn't nice to model features as an enum. The optimal way to represent these is a bitset (not necessarily recommending it, it doesn't matter in this case)`.
You'd have something like:
if viewModel.supports(.featuredImage)| return settings.slug | ||
| } | ||
| if let abstractPost { | ||
| return abstractPost.suggested_slug ?? "" |
There was a problem hiding this comment.
The changes to PostSettingsViewModel are the only ones that look quite risky. It changes the existing code and introduces multiple if. I would suggest adding a protocol for PostSettingsViewModel and providing two separate implementation: one for Core Data (with no changes) and one for wordpress-rs. It should be easier to maintain and would reduce the risk of regressions. It's a classic refactoring strategy that should be helpful in other places when integrating wp-rs.
There was a problem hiding this comment.
The reason I used an enum is to make code review easier. The code movement is clearer in the diff. But I should have changed the if-else statements to switch-case, which is done in 4f0477b.
I agree with you that using a protocol would make the code nicer, which I can do after the chained PR #25274 is merged. I hope that's okay with you?
There was a problem hiding this comment.
That works, thanks! Ideally, I'd suggest using branch-by-abstraction as much as possible. It'll make it easier to review, reduce risk of regressions, and make our life easier once we are ready to remove the Core Data based types.
|
I'd also suggest running I smoke-tested the changes on regular posts. |
crazytonyli
force-pushed
the
task/post-settings-refactor
branch
from
c482a6f to
07358d1
Compare
|
I have run the Architecture & Design
I'll ignore this one.
That's a good point, I probably could/should bind the CustomPostEditorService to the main actor. There will be further changes to CustomPostEditorService, so I'll keep this note in mind. Specific Issues
The comments are intentional, and some of them are addressed in #25274
Again, these comments are intentional.
Again, it's intentional and is addressed in #25274
Seeing "-" is not ideal. We can proactively fetch author again, instead of just quering
May be it's not obvious, but this is noted in the
Again, it's covered by the code comment. Testing
Deleted. Minor Nits
Indeed. Removed. |
I keep forgetting that LLMs generate different output each time 🤦 I'm pretty sure my review items were different. |
|
Approved, left a couple of additional replies. |
|
|
Version |
4 participants
Note
This PR will be merged after #25267.
Description
This PR introduces Post Settings and the Publish Sheet for custom posts, both under the same feature flag. There are a few features (adding terms, updating parent post) that are not implemented yet. But the UI is there, and the main refactor is complete and ready for review.
The existing Post Settings are reused, and the main changes are in the
PostSettingsandPostSettingsViewModel, which are updated to supportAnyPostWithEditContext.The commits are individually reviewable. But I can't say that's necessarily easier than reviewing the changes as a whole.
Testing instructions
The most important test is probably the regression post settings on Posts and Pages.