Fix certain M1 settings aren't accessible if the data are empty when M2 feature switch is off #2303

jaclync · 2020-05-15T09:55:49Z

Changes

Since there 2 places that require the knowledge of the visible actions on the product form bottom sheet (the CTA visibility + bottom sheet list selector data), the duplicated actions logic from ProductFormViewController and ProductFormBottomSheetListSelectorCommand were now shared in ProductFormBottomSheetActionsFactory
Updated ProductFormViewController to get the bottom sheet actions from ProductFormBottomSheetActionsFactory and passing them to ProductFormBottomSheetListSelectorCommand
Updated unit tests

Testing

Prerequisite: prepare a simple product that is not virtual/downloadable (so that it has shipping settings) and its shipping data are all empty

Go to Settings > Experimental Features, and turn off the Products feature switch
Go to the Products tab
Tap on the simple Product in the prerequisite --> "Add more details" CTA should be visible at the bottom

Example screenshots

before (no way to edit shipping settings)	after

Update release notes:

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

…ct `ProductFormBottomSheetActionsFactory`. Move existing unit tests to the factory.

pmusolino

Hey Jaclyn! During the test, I encountered a crash tapping on the product "Premium Quality" on my test store on an iPhone Xs with iOS 12.2 (it happens also on other products every time I launch the application from Xcode)

I also encountered that the content of the cells now it's not populated (this happens for every simple product that I'm able to open):

I've also 4 tests failing.

jaclync · 2020-05-18T00:50:00Z

Thanks for testing @pmusolino ! I just tested your store on iPhone Xs simulator iOS 12.1, and somehow I wasn't able to see the crashes and issues you had with M2 feature flag off or on 🤔also, I was wondering why your test cases looked different:

test cases that failed during your testing	test cases that passed on my end

I was able to open all the products on your store:

Premium Quality product	"Woo Ninja - H&M" product

It'd be great if I can know more about a few things for further investigation on my side:

for the crash, do you mind sharing the indexPath.row value?
it sounds like you were able to reproduce the crash/label issue consistently? were you able to reproduce on all devices, or specific to this OS/device type?
did you have any diffs during your testing, like to manually turn on/off other feature flags? I tested with M3 flag on/off manually in DefaultFeatureFlagService and both worked for me

jaclync · 2020-05-18T01:33:27Z

this PR is going to target release 4.3 for Products M2 release behind a feature switch, sorry if it's not making the code freeze today! cc @oguzkocer

pmusolino · 2020-05-18T11:27:48Z

I'm unlucky. After a logout, I'm no more able to replicate the crash or the failing tests. :(
I can't really tell you what was different. The M2 product flag was disabled (from the Experimental Features screen), while the M3 one was enabled.

pmusolino · 2020-05-18T11:54:44Z

@shiki Can you take a look at it too? There is a crash that I can no longer replicate, but I am afraid to send something into production that could have a crash in the products. 🤕

shiki

LGTM. 😬 I investigated the code and I can't seem to find a reason why the crash that Paolo shared would happen.

...ted/Products/Edit Product/BottomSheetListSelector/ProductFormBottomSheetActionsFactory.swift

shiki · 2020-05-18T16:21:18Z

...ted/Products/Edit Product/BottomSheetListSelector/ProductFormBottomSheetActionsFactory.swift

+            shouldShowCategoriesRow ? .editCategories: nil,
+            shouldShowShortDescriptionRow ? .editBriefDescription: nil
+        ]
+        return actions.compactMap({ $0 }).filter({ $0.isVisible(product: product) })


I wonder if we should also encapsulate the isVisible logic inside this Factory. 🤔 Though that is also eventually used within DefaultProductFormTableViewModel.settingsRows. This makes me think that all these visibility logic should be merged too.

I wonder if we've outgrown the current architecture (expected and normal) and we need to do a review at some point in the future.

yea I think it'd be nice if we can merge the logic for making the bottom sheet actions and settings rows since they are based on the same data (product editing categories), maybe having both created from the same factory (a different name like ProductFormActionsFactory). what do you think? I created an issue for tech debt #2317 to track 🙂

shiki · 2020-05-18T16:23:38Z

WooCommerce/Classes/ViewRelated/Products/Edit Product/ProductFormViewController.swift

+        let actions = ProductFormBottomSheetActionsFactory.actions(product: product,
+                                                                   isEditProductsRelease2Enabled: isEditProductsRelease2Enabled,
+                                                                   isEditProductsRelease3Enabled: isEditProductsRelease3Enabled)


I'd suggest making a private helper:

private func makeBottomSheetActions() -> [ProductFormBottomSheetAction] { ProductFormBottomSheetActionsFactory.actions(product: product, isEditProductsRelease3Enabled: isEditProductsRelease3Enabled) { [weak self] action in isEditProductsRelease2Enabled: isEditProductsRelease2Enabled, isEditProductsRelease3Enabled: isEditProductsRelease3Enabled).actions }

And then we could use that in updateMoreDetailsButtonVisibility too.

updated in 768fd0f (I named it createBottomSheetActions() to be consistent with other create* functions)

shiki · 2020-05-18T16:26:40Z

...ted/Products/Edit Product/BottomSheetListSelector/ProductFormBottomSheetActionsFactory.swift

+
+struct ProductFormBottomSheetActionsFactory {
+    /// Retruns an array of actions that are visible in the product form bottom sheet.
+    static func actions(product: Product, isEditProductsRelease2Enabled: Bool, isEditProductsRelease3Enabled: Bool) -> [ProductFormBottomSheetAction] {


Not really significant for this but it'd be nice if we avoid static methods as much as possible. For Factory objects for example, they can be injected for mocking and non-static methods would be better for that use case.

let factory = ProductFormBottomSheetActionsFactory(product: product, ...) let editForm = ProductFormVC(bottomSheetActionsFactory: factory)

I usually prefer not to use static methods either, but wasn't exactly sure what our conventions are since we also have quite some static methods in the codebase 🤔 On the other hand, if we are good with the plan in #2317 to have the factory create actions for both the bottom sheet and product form, it'd be nice to pass all the dependencies to the initializer and we can just get the bottom sheet actions (and product form actions in the future) directly. I made an update in 353fd12

oguzkocer · 2020-05-18T19:45:29Z

@jaclync Thank you for the heads up, really appreciate it 🙇‍♂️ I've updated the base branch to the frozen release/14.3. I'll release the first beta once this PR is merged.

… to instance with dependencies passed to initializer.

peril-woocommerce · 2020-05-19T00:31:14Z

You can trigger an installable build for these changes by visiting CircleCI here.

peril-woocommerce · 2020-05-19T00:31:14Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

jaclync

thanks to @pmusolino and @shiki for double testing and reviewing! I'm also worried about the crashes and visual issues that Paolo encountered, even though I haven't seen them before and I tried with many stores yesterday and couldn't reproduce. It's certainly something to watch out for during internal testing.

@shiki I responded to the CR comments and merging this PR now to unblock the beta build, but I'm happy to address anything separately!

@oguzkocer thanks a lot for updating the PR base! I believe we are good for internal testing for release 4.3 now 👍

jaclync added 2 commits May 15, 2020 17:43

Move product form bottom sheet actions calculation to a separate stru…

011c0c2

…ct `ProductFormBottomSheetActionsFactory`. Move existing unit tests to the factory.

Add test cases when both release 2 and 3 features are off.

1a52c63

jaclync added type: bug A confirmed bug. feature: add/edit products Related to adding or editing products. labels May 15, 2020

jaclync added this to the 4.3 milestone May 15, 2020

jaclync requested a review from a team May 15, 2020 09:55

jaclync self-assigned this May 15, 2020

pmusolino suggested changes May 15, 2020

View reviewed changes

pmusolino self-requested a review May 18, 2020 11:15

pmusolino requested a review from shiki May 18, 2020 11:51

shiki self-assigned this May 18, 2020

shiki approved these changes May 18, 2020

View reviewed changes

oguzkocer changed the base branch from develop to release/4.3 May 18, 2020 19:43

jaclync added 3 commits May 19, 2020 08:22

Add struct level comment.

5a04581

Add a function that creates bottom sheet actions.

768fd0f

Update ProductFormBottomSheetActionsFactory's actions from static…

353fd12

… to instance with dependencies passed to initializer.

jaclync commented May 19, 2020

View reviewed changes

jaclync merged commit f988e8a into release/4.3 May 19, 2020

jaclync deleted the issue/2302-m1-settings-missing branch May 19, 2020 00:47

oguzkocer mentioned this pull request May 21, 2020

Merge 4.3 code freeze and release notes to develop #2331

Merged

Fix certain M1 settings aren't accessible if the data are empty when M2 feature switch is off #2303

Fix certain M1 settings aren't accessible if the data are empty when M2 feature switch is off #2303

Uh oh!

Conversation

jaclync commented May 15, 2020

Changes

Testing

Example screenshots

Uh oh!

pmusolino left a comment

Choose a reason for hiding this comment

Uh oh!

jaclync commented May 18, 2020

Uh oh!

jaclync commented May 18, 2020

Uh oh!

pmusolino commented May 18, 2020

Uh oh!

pmusolino commented May 18, 2020

Uh oh!

shiki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shiki May 18, 2020

Choose a reason for hiding this comment

Uh oh!

jaclync May 19, 2020

Choose a reason for hiding this comment

Uh oh!

shiki May 18, 2020

Choose a reason for hiding this comment

Uh oh!

jaclync May 19, 2020

Choose a reason for hiding this comment

Uh oh!

shiki May 18, 2020

Choose a reason for hiding this comment

Uh oh!

jaclync May 19, 2020

Choose a reason for hiding this comment

Uh oh!

oguzkocer commented May 18, 2020

Uh oh!

peril-woocommerce bot commented May 19, 2020

Uh oh!

peril-woocommerce bot commented May 19, 2020

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants