Decouple Blog from JetpackFeaturesRemovalCoordinator - #24393
Conversation
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 27055 | |
| Version | PR #24393 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | a51b038 | |
| Installation URL | 5e1sq0jfmlvtg |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 27055 | |
| Version | PR #24393 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | a51b038 | |
| Installation URL | 6s3scgaoh9u70 |
| case BlogFeatureStockPhotos: | ||
| return [self supportsRestApi] && [JetpackFeaturesRemovalCoordinator jetpackFeaturesEnabled]; | ||
| return [self supportsStockPhotos]; | ||
| case BlogFeatureTenor: | ||
| return [JetpackFeaturesRemovalCoordinator jetpackFeaturesEnabled]; | ||
| return [self supportsTenor]; |
There was a problem hiding this comment.
As an aside, I'm not sure how useful it is for us to have all this logic living in the Blog model, in particular checks such as whether the app is Jetpack.
We could explode this method in per-feature ones that take a Blog and the relevant app state as input and return true or false.
kean
left a comment
kean
left a comment
There was a problem hiding this comment.
Hey, I opened a PR that moves JetpackFeaturesRemovalCoordinator usages to the app #24395. I'd suggest keeping any app-specific checks out of Blog or WordPressData in general.
In the future, we'll might want to introduce another abstraction for features or somehow integrate it with FeatureFlag, but this will do for now.
| XCTAssertTrue(blog.supportsRestApi()) | ||
| } | ||
|
|
||
| func testSupportsStockPhotos() { |
There was a problem hiding this comment.
I'd suggest avoiding writing tests for configuration. In this case, the tests are more complicated than the implementation and match the logic one-to-one.
|
Closing in favor of #24395
Yeah. I thought that too, I was coming the point of view of just getting this to compile. But I appreciate you going the extra mile and making the design better for the future. 🙇♂️ |
4 participants
I don't remember where, but we discussed that it was safe to replace
[JetpackFeaturesRemovalCoordinator jetpackFeaturesEnabled]inBlogwith a check forBuildSettings.current.brand == .jetpack.However,
BuildSettingsis only available to Swift, which means moving the logic that accessesJetpackFeaturesRemovalCoordinatorto Swift. I started down that route in #24332 but I run into UI test failures and got worried about the surface area and combine lack of unit tests.This PR targets only the
JetpackFeaturesRemovalCoordinatordecoupling, and with tests.Notice that to do this I added a fixture extension to
BuildSettings, so we can create ad hoc values in the unit tests to inject.Part of #24165.