[Shipping labels] Get account settings and use in UI custom packages #14499

bozidarsevo · 2024-11-22T15:21:12Z

Closes #14484

Description

Loading store options from backend so it can be used when creating custom packages (for weight and dimension units)
I have a work in progress branch where I have been working on logic for persisting the store options in Core Data but I am wondering if we should do it or not? The main thing I am thinking about is whether we always need "fresh" store options or not, if we need fresh ones then the persisting them and loading them does not make much sense since we will need to get the fresh ones. @rachelmcr what do you think?

Testing information

Install and set up the Woo Shipping extension on your store.
Build and run the app with the revampedShippingLabelCreation feature flag enabled.
Create an order with the processing status and at least one physical product.
In the order details, select "Create Shipping Label."
Store options will start fetching from backend in the background.
Tap "Select a Package" button
If store options are already loaded you will see the UI, if not you will see the loading indicator until the loading finishes. If it fails you will see a reload button that triggers the loading again.
Check that the fields have proper weight and dimensions units (turn template switch ON to see weight unit).

Screenshots

Flow when store options are not loaded yet and users taps fast thru UI and gets to UI part that needs it:

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-11-29.at.11.10.27.mp4

Store options already loaded and UI has everything needed:

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-11-29.at.11.10.37.mp4

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

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

bozidarsevo · 2024-11-22T15:33:02Z

...ing Labels/WooShipping Package and Rate Selection/WooShippingAddCustomPackageViewModel.swift

@@ -141,6 +139,24 @@ final class WooShippingAddCustomPackageViewModel: ObservableObject {
        }
        return true
    }
+
+    func loadStoreOptions() {


Loading and using the store options so we get fresh/live weight and dimensions units.

What should we do if we do not get them, should we show some kind of error and ability to load again? I added a simple fresh button to trigger it again. In a separate PR I am preparing the logic for persisting that data in CoreData but I am wondering if we should use persisted/saved data or always fetch fresh?

wpmobilebot · 2024-11-22T15:33:12Z

📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS
	Build Number	pr14499-7a41f8c
	Version	21.2
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`7a41f8c`
	App Center Build	WooCommerce - Prototype Builds #11913

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

…packages

bozidarsevo · 2024-11-25T08:53:50Z

... Create Shipping Labels/WooShipping Package and Rate Selection/WooAddCustomPackageView.swift

-            ScrollViewReader { proxy in
-                ScrollView {
-                    VStack(alignment: .leading, spacing: Constants.defaultVerticalSpacing) {
-                        HStack {
-                            Text(Localization.packageType)
-                                .font(.subheadline)
-                            Spacer()
-                        }
-                        packageTypeSelectionView
-                        VStack {
-                            AdaptiveStack(spacing: 8) {
-                                ForEach(WooShippingPackageUnitType.dimensionUnits, id: \.self) { dimensionUnit in
-                                    unitInputView(for: dimensionUnit, unit: customPackageViewModel.dimensionsUnit)
+            if let storeOptions = customPackageViewModel.storeOptions {
+                ScrollViewReader { proxy in
+                    ScrollView {
+                        VStack(alignment: .leading, spacing: Constants.defaultVerticalSpacing) {
+                            HStack {
+                                Text(Localization.packageType)
+                                    .font(.subheadline)
+                                Spacer()
+                            }


This part looks like a lot of code change but basically if/else is added to show the content if the store options are loaded and show loading view if not.

…packages

rachelmcr · 2024-11-26T18:11:24Z

From the discussion in Slack (p1732618862774319-slack-C02KUCFCSFP):

The idea is to fetch these settings in the view model for the main purchase screen once (every time the label creation flow is started).
Then we can hold them in memory (rather than persisting them in the storage layer) and pass them to the view models that need those settings.

@bozidarsevo let me know when you're ready for me to take another look at this! Thanks for raising the question about storage. 👍

FWIW, I think if we fail to fetch them from remote at this point we should just fall back to the store settings, which are updated each time the app launches.

bozidarsevo · 2024-11-27T08:23:42Z

@bozidarsevo let me know when you're ready for me to take another look at this! Thanks for raising the question about storage. 👍

Sure! I will do few updates regarding those couple of things and then reassign when PR is ready for another look. Thanks!

…packages # Conflicts: # WooCommerce/Classes/ViewRelated/Orders/Order Details/Shipping Labels/WooShipping Create Shipping Labels/WooShippingCreateLabelsViewModel.swift

…packages

bozidarsevo · 2024-11-27T13:24:34Z

...Details/Shipping Labels/WooShipping Create Shipping Labels/WooShippingCreateLabelsView.swift

+    }
+
+    @ViewBuilder
+    private var expandableBottomSheet: some View {


Separated UI code into helper properties so it can be updated easier in the future

bozidarsevo · 2024-11-27T13:25:30Z

...ls/Shipping Labels/WooShipping Create Shipping Labels/WooShippingCreateLabelsViewModel.swift

+    @Published var storeOptions: ShippingLabelStoreOptions?
+    @Published var isLoadingStoreOptions: Bool = false
+
+    func loadStoreOptions() {


Loading and storing the store options so we get fresh/live weight and dimensions units.

That property is then being passed to all UI in shipping labels flow

bozidarsevo · 2024-11-27T13:25:47Z

...Details/Shipping Labels/WooShipping Create Shipping Labels/WooShippingCreateLabelsView.swift

@@ -37,109 +37,37 @@ struct WooShippingCreateLabelsView: View {
    var body: some View {
        NavigationStack {
            ScrollView {
-                VStack(spacing: Layout.verticalSpacing) {


This part looks like a lot of code change but basically if/else is added to show the content if the store options are loaded and show loading view if not.

…packages

rachelmcr · 2024-11-28T12:00:01Z

@bozidarsevo I'm having some trouble following all of the changes in this PR. In particular, I appreciate having expandableBottomSheet moved into a separate property but it makes it hard to distinguish between the changes made just to reorganize the code vs. the functional changes made for loading the store options here. Could you split this PR into smaller ones or perhaps just split at least that commit, so the changes are easier to review? Thanks!

bozidarsevo · 2024-11-28T12:53:59Z

@bozidarsevo I'm having some trouble following all of the changes in this PR. In particular, I appreciate having expandableBottomSheet moved into a separate property but it makes it hard to distinguish between the changes made just to reorganize the code vs. the functional changes made for loading the store options here. Could you split this PR into smaller ones or perhaps just split at least that commit, so the changes are easier to review? Thanks!

I will try something. 👍
The body of that view has a lot of lines so just adding one if statement produces a big code difference, but I will make it simpler.

bozidarsevo · 2024-11-28T13:04:01Z

...Details/Shipping Labels/WooShipping Create Shipping Labels/WooShippingCreateLabelsView.swift

@@ -56,7 +56,12 @@ struct WooShippingCreateLabelsView: View {
                        WooShippingServiceView(viewModel: shippingService)
                            .padding(.horizontal, -16)
                    } else {
-                        WooShippingPackageAndRatePlaceholder()
+                        if let storeOptions = viewModel.storeOptions {


This is now much cleaner and easier to review 🥳
I put the check for the storeOptions just here so that just this view is not showed before we get storeOptions so later on we can do other changes if needed 🕺 @rachelmcr

…packages

rachelmcr

I'm approving this PR because all of these changes work as intended and, given our sync discussion, I don't want to block us from moving forward.

However, I don't think the loading indicator is a great experience on the main screen here. I think it's worth taking a different approach that will allow us to immediately load the settings we already have for the store and refresh them in place. That said, I'm ok if you want to merge these changes now and revisit the behavior after M1 is complete and we do a full design review.

rachelmcr · 2024-11-28T13:19:55Z

... Create Shipping Labels/WooShipping Package and Rate Selection/WooAddCustomPackageView.swift

@@ -1,4 +1,5 @@
 import SwiftUI
+import enum Yosemite.WooShippingAction


nit: This import isn't needed in the view here

rachelmcr · 2024-11-28T13:22:47Z

...ing Labels/WooShipping Package and Rate Selection/WooShippingAddCustomPackageViewModel.swift

-        self.weightUnit = weightUnit ?? ""
+         storeOptions: ShippingLabelStoreOptions,
+         stores: StoresManager = ServiceLocator.stores) {
+        self.storeOptions = storeOptions


nit: What do you think of keeping the dimensionsUnit and weightUnit properties and just using the ShippingLabelStoreOptions to set those values? No worries if you want to leave it like this, but the other approach felt a little clearer to me.

After reading through the changes in WooShippingCreateLabelsViewModel I'm more inclined to keep these properties and the init parameters the same here. That way we only need to pass in the data that we're actually using, which can simplify the fallbacks we need to set if the request to load the store options fails. Just a thought, not a blocking suggestion!

rachelmcr · 2024-11-28T15:02:24Z

...Details/Shipping Labels/WooShipping Create Shipping Labels/WooShippingCreateLabelsView.swift

+                            WooShippingPackageAndRatePlaceholder(storeOptions: storeOptions)
+                        }
+                        else {
+                            loadingView


Having a loading view here feels a little odd to me, especially because the data isn't needed for this placeholder view — it just needs to be passed down to its child views.

What do you think about setting the store options as an environment variable, instead? That way we could initialize this view without the storeOptions property necessarily being loaded yet, and once we have those options we can pass them to the views that need them. Or we could use some other approach to set/update the store options after initialization.

This would also allow us to use the fallbacks from ServiceLocator immediately, and then update them in place if we get fresh data from the backend.

We will still need to do pick a place where we want to trigger the fetching/loading the data. And in theory that loading could take a longer time and user could get to a state in UI that needs this data and we need to show that something will be there.

For example, I do not show any loading indicators and user gets to Add Package screen and Custom tab. I still do not have the store options therefor I do not have the dimension and weight units. Should I then show some indicator that something is loading? Because at that state we do not have all the data needed to show the UI and we cannot just show blank white screen.

If I use environment variable again we need to decide when will we trigger fetching/loading so it would basically be the same just a different implementation.

I can do it in a way that I trigger the initial load when we enter label creation flow and then if some part of the app needs the store options and they are not loaded yet I will show some loading indication there, basically showing the loading at the last possible state to avoid blocking user as little as possible.

I will see if I will do that change here or in a separate PR when I see how big of a change would it be.

Updated:

Basically loading store options (account settings) when we enter the label creation screen, but showing loading indicator only if I get to the part of the UI that needs the store options and they are not loaded. That way in most cases users will not even see the loading indicator. 😊

Updated screen recordings in description

For example, I do not show any loading indicators and user gets to Add Package screen and Custom tab. I still do not have the store options therefor I do not have the dimension and weight units. Should I then show some indicator that something is loading?

Why not use the dimension and weight units from ServiceLocator.shippingSettingsService by default? We can still trigger fetching/loading the account settings when the shipping label flow begins, and update those units in place if they are different from the default we've used.

bozidarsevo · 2024-11-29T10:44:06Z

...reate Shipping Labels/WooShipping Package and Rate Selection/WooShippingAddPackageView.swift

@@ -60,6 +73,18 @@ struct WooShippingAddPackageView: View {
        .task {
            packagesRepository.loadPackages()
        }
+        .onAppear() {
+            if let storeOptions = createLabelsViewModel.storeOptions {


If there is storeOptions at appear we use it, if not we check for updates for storeOptions and update when we receive changes for it.

…packages

bozidarsevo added 2 commits November 22, 2024 16:15

Load store options for custom package creation

ba8743a

Update WooShippingAddCustomPackageViewModelTests.swift

a18d6f1

bozidarsevo added the feature: shipping labels Related to creating, ordering, or printing shipping labels. label Nov 22, 2024

bozidarsevo added this to the 21.3 milestone Nov 22, 2024

bozidarsevo commented Nov 22, 2024

View reviewed changes

bozidarsevo added 3 commits November 22, 2024 16:33

Update WooAddCustomPackageView.swift

a037029

Update WooShippingAddCustomPackageViewModelTests.swift

a8368a2

Merge branch 'trunk' into issue/14484-account-settings-for-UI-custom-…

cc3ec75

…packages

bozidarsevo requested a review from rachelmcr November 25, 2024 08:52

bozidarsevo commented Nov 25, 2024

View reviewed changes

bozidarsevo marked this pull request as ready for review November 25, 2024 08:56

bozidarsevo added 3 commits November 25, 2024 15:10

Merge branch 'trunk' into issue/14484-account-settings-for-UI-custom-…

55cf241

…packages

Update after merge conflict

187c375

Update WooShippingAddCustomPackageViewModel.swift

6e8ea7c

bozidarsevo mentioned this pull request Nov 26, 2024

Remove WooShippingPackagesRepository #14519

Merged

4 tasks

bozidarsevo added 6 commits November 27, 2024 09:49

Load store options in WooShippingCreateLabelsViewModel

ac1f13d

Merge branch 'trunk' into issue/14484-account-settings-for-UI-custom-…

893a098

…packages # Conflicts: # WooCommerce/Classes/ViewRelated/Orders/Order Details/Shipping Labels/WooShipping Create Shipping Labels/WooShippingCreateLabelsViewModel.swift

Update WooShippingAddCustomPackageViewModel.swift

4d4499c

Update WooShippingCreateLabelsView.swift

c046426

Update WooShippingCreateLabelsViewModel.swift

f592b08

Update WooShippingAddCustomPackageViewModelTests.swift

9d0a13d

rachelmcr removed their request for review November 27, 2024 10:27

Merge branch 'trunk' into issue/14484-account-settings-for-UI-custom-…

38ceb16

…packages

bozidarsevo commented Nov 27, 2024

View reviewed changes

bozidarsevo requested a review from rachelmcr November 27, 2024 13:25

bozidarsevo added 4 commits November 27, 2024 14:43

Remove packagesRepository from the model

94a57cb

Update WooShippingAddCustomPackageViewModelTests.swift

e12987f

[Shipping labels] Custom packages model update (#14540)

21c746d

Merge branch 'trunk' into issue/14484-account-settings-for-UI-custom-…

5433fc5

…packages

rachelmcr self-assigned this Nov 28, 2024

bozidarsevo added 2 commits November 28, 2024 14:01

Update WooShippingCreateLabelsView.swift

7327882

Update WooShippingCreateLabelsView.swift

1aaccc0

bozidarsevo commented Nov 28, 2024

View reviewed changes

Merge branch 'trunk' into issue/14484-account-settings-for-UI-custom-…

bc134ba

…packages

rachelmcr approved these changes Nov 28, 2024

View reviewed changes

rachelmcr mentioned this pull request Nov 28, 2024

[Shipping labels] Set selected package from "Add Package" flow #14558

Merged

4 tasks

bozidarsevo added 4 commits November 29, 2024 10:12

Use units directly in WooShippingAddCustomPackageViewModel

5c3fbfb

Updates to store options loading logic

7eeb80e

Update WooShippingAddCustomPackageViewModel.swift

a910f78

Update WooShippingPackageAndRatePlaceholder.swift

c17af3e

bozidarsevo commented Nov 29, 2024

View reviewed changes

bozidarsevo added 8 commits November 29, 2024 11:48

Remove not needed callback

b500b78

Merge branch 'trunk' into issue/14484-account-settings-for-UI-custom-…

66705df

…packages

Merge branch 'trunk' into issue/14484-account-settings-for-UI-custom-…

103ba97

…packages

Merge branch 'trunk' into issue/14484-account-settings-for-UI-custom-…

191901b

…packages

Update WooShippingPackageAndRatePlaceholder.swift

31b1e6f

Update WooShippingPackageAndRatePlaceholder.swift

64c956b

Merge branch 'trunk' into issue/14484-account-settings-for-UI-custom-…

04fcea8

…packages

Update WooShippingPackageAndRatePlaceholder.swift

7a41f8c

bozidarsevo merged commit 700f138 into trunk Dec 2, 2024
14 checks passed

bozidarsevo deleted the issue/14484-account-settings-for-UI-custom-packages branch December 2, 2024 17:39

bozidarsevo changed the title ~~Get account settings and use in UI custom packages~~ [Shipping labels] Get account settings and use in UI custom packages Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Shipping labels] Get account settings and use in UI custom packages #14499

[Shipping labels] Get account settings and use in UI custom packages #14499

bozidarsevo commented Nov 22, 2024 •

edited

Loading

bozidarsevo Nov 22, 2024

wpmobilebot commented Nov 22, 2024 •

edited

Loading

bozidarsevo Nov 25, 2024

rachelmcr commented Nov 26, 2024

bozidarsevo commented Nov 27, 2024

bozidarsevo Nov 27, 2024

bozidarsevo Nov 27, 2024

bozidarsevo Nov 27, 2024

rachelmcr commented Nov 28, 2024

bozidarsevo commented Nov 28, 2024

bozidarsevo Nov 28, 2024

rachelmcr left a comment

rachelmcr Nov 28, 2024

rachelmcr Nov 28, 2024

rachelmcr Nov 28, 2024

rachelmcr Nov 28, 2024

bozidarsevo Nov 29, 2024

bozidarsevo Nov 29, 2024

rachelmcr Nov 29, 2024

bozidarsevo Nov 29, 2024

		@@ -1,4 +1,5 @@
		import SwiftUI
		import enum Yosemite.WooShippingAction

[Shipping labels] Get account settings and use in UI custom packages #14499

[Shipping labels] Get account settings and use in UI custom packages #14499

Conversation

bozidarsevo commented Nov 22, 2024 • edited Loading

Description

Testing information

Screenshots

Reviewer (or Author, in the case of optional code reviews):

Choose a reason for hiding this comment

wpmobilebot commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

rachelmcr commented Nov 26, 2024

bozidarsevo commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachelmcr commented Nov 28, 2024

bozidarsevo commented Nov 28, 2024

Choose a reason for hiding this comment

rachelmcr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bozidarsevo commented Nov 22, 2024 •

edited

Loading

wpmobilebot commented Nov 22, 2024 •

edited

Loading