Blaze: Ad destination parameters #10762

0nko · 2024-02-09T20:34:57Z

Implements #10737, a subtask of #10665.

Screen_recording_20240209_213359.webm

To test:

Apply the patch below to see any parameters
Create a new Blaze campaign
Tap on the Ad destination property
Notice the base URL and parameters are printed out
Tap on the URL parameters property
Notice the 2 parameters are displayed in a list
Notice a full URL is shown below the list
Delete one parameter
Notice the full URL and remaining character count is updated
Tap on back
Notice the parameters property got updated

Patch:

Index: WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/preview/BlazeCampaignCreationPreviewFragment.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/preview/BlazeCampaignCreationPreviewFragment.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/preview/BlazeCampaignCreationPreviewFragment.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/preview/BlazeCampaignCreationPreviewFragment.kt	(revision Staged)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/preview/BlazeCampaignCreationPreviewFragment.kt	(date 1707510168898)
@@ -77,7 +77,7 @@
                 is NavigateToAdDestinationScreen -> findNavController().navigateSafely(
                     BlazeCampaignCreationPreviewFragmentDirections
                         .actionBlazeCampaignCreationPreviewFragmentToBlazeCampaignCreationAdDestinationFragment(
-                            event.targetUrl,
+                            event.targetUrl + "?test=true&yes=no",
                             event.productId
                         )
                 )

dangermattic · 2024-02-09T20:35:23Z

	3 Warnings
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	Class `BlazeCampaignCreationAdDestinationParametersViewModel` is missing tests, but `unit-tests-exemption` label was set to ignore this.
⚠️	Class `ViewState` is missing tests, but `unit-tests-exemption` label was set to ignore this.

Generated by 🚫 Danger

wpmobilebot · 2024-02-09T20:53:53Z

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

	App Name	WooCommerce Android
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`7c63302`
	Direct Download	`woocommerce-prototype-build-pr10762-7c63302.apk`

JorgeMucientes · 2024-02-13T09:11:54Z

.../android/ui/blaze/creation/destination/BlazeCampaignCreationAdDestinationParametersScreen.kt

+            item(key = "footer") {
+                Column(
+                    modifier = Modifier
+                        .animateItemPlacement()
+                        .fillMaxWidth()
+                ) {


Out of curiosity why did you add the footer inside the lazy column? Why not just below it?

Because of the animation. When I put it outside, the lazy column items are animated but the footer jumps.

JorgeMucientes · 2024-02-13T09:13:47Z

...droid/ui/blaze/creation/destination/BlazeCampaignCreationAdDestinationParametersViewModel.kt

+    savedStateHandle: SavedStateHandle
+) : ScopedViewModel(savedStateHandle) {
+    companion object {
+        // The maximum number of characters allowed in a URL by Chrome


Are we following the Chrome restrictions in the end or the Calypso web restrictions? Just double checking after seeing this convo: p1707498869846739-slack-C03L1NF1EA3

Yes, I've updated the behavior to match the web/iOS.

JorgeMucientes · 2024-02-13T09:29:05Z

...ommerce/android/ui/blaze/creation/destination/BlazeCampaignCreationAdDestinationViewModel.kt

+    private fun getParameters(url: String): String {
+        return url.split("?")
+            .getOrNull(1)
+            ?.replace("&", "\n")
+            ?: return resourceProvider.getString(R.string.blaze_campaign_edit_ad_destination_empty_parameters_message)
+    }


Minor suggestion, you can maybe reuse the parseParameters() function to make it slightly "more readable"?

private fun getParameters(url: String): String { return url.parseParameters().entries.joinToString(separator = "\n") .ifBlank { resourceProvider.getString(R.string.blaze_campaign_edit_ad_destination_empty_parameters_message) } }

It's more of a personal taste thing, so feel free to ignore this.

Thanks for the suggestion, I updated the code.

JorgeMucientes · 2024-02-13T09:30:08Z

Great work @0nko. Everything works as expected and code looks good! I just left a couple questions, but aside from that this si ready to be merged.

peril-woocommerce · 2024-02-13T10:50:21Z

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

Generated by 🚫 dangerJS

codecov-commenter · 2024-02-13T10:50:39Z

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (3c97bdf) 41.44% compared to head (7c63302) 41.41%.
Report is 97 commits behind head on trunk.

Files	Patch %	Lines
...ampaignCreationAdDestinationParametersViewModel.kt	0.00%	21 Missing ⚠️
...ion/BlazeCampaignCreationAdDestinationViewModel.kt	0.00%	21 Missing ⚠️
...in/kotlin/com/woocommerce/android/util/UrlUtils.kt	0.00%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #10762      +/-   ##
============================================
- Coverage     41.44%   41.41%   -0.04%     
  Complexity     5017     5017              
============================================
  Files          1016     1017       +1     
  Lines         58364    58407      +43     
  Branches       7803     7806       +3     
============================================
  Hits          24190    24190              
- Misses        32030    32073      +43     
  Partials       2144     2144

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

0nko · 2024-02-13T10:51:16Z

@JorgeMucientes Thanks for the review. I've addressed your feedback.

0nko added 7 commits February 9, 2024 15:34

Add destination URL parameters screen boilerplate

06989e0

Add string resources

78f6dad

Add parameter handling logic to ad destination view model

9e010ba

Add navigation and result handling

5afedb5

Add URL manipulation util extension functions

1b7093f

Update state change management when parameters are updated

dd5fa4b

Fix ktlint issues

97cc4f0

0nko added feature: blaze Related to the Blaze project unit-tests-exemption labels Feb 9, 2024

0nko added this to the 17.4 milestone Feb 9, 2024

0nko requested review from hichamboushaba and JorgeMucientes February 9, 2024 20:34

JorgeMucientes self-assigned this Feb 12, 2024

JorgeMucientes approved these changes Feb 13, 2024

View reviewed changes

0nko added 3 commits February 13, 2024 11:20

Update the remaining character count logic to match the web

8bde2d2

Make the getParameters more readable

216cd1a

Remove the unneeded extension function

7c63302

JorgeMucientes merged commit c2ab3b0 into trunk Feb 13, 2024
15 checks passed

JorgeMucientes deleted the issue/10737-ad-url-params branch February 13, 2024 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blaze: Ad destination parameters #10762

Blaze: Ad destination parameters #10762

0nko commented Feb 9, 2024

dangermattic commented Feb 9, 2024

wpmobilebot commented Feb 9, 2024 •

edited

Loading

JorgeMucientes Feb 13, 2024

0nko Feb 13, 2024

JorgeMucientes Feb 13, 2024

0nko Feb 13, 2024

JorgeMucientes Feb 13, 2024

0nko Feb 13, 2024

JorgeMucientes commented Feb 13, 2024

peril-woocommerce bot commented Feb 13, 2024

codecov-commenter commented Feb 13, 2024 •

edited

Loading

0nko commented Feb 13, 2024

Blaze: Ad destination parameters #10762

Blaze: Ad destination parameters #10762

Conversation

0nko commented Feb 9, 2024

dangermattic commented Feb 9, 2024

wpmobilebot commented Feb 9, 2024 • edited Loading

JorgeMucientes Feb 13, 2024

Choose a reason for hiding this comment

0nko Feb 13, 2024

Choose a reason for hiding this comment

JorgeMucientes Feb 13, 2024

Choose a reason for hiding this comment

0nko Feb 13, 2024

Choose a reason for hiding this comment

JorgeMucientes Feb 13, 2024

Choose a reason for hiding this comment

0nko Feb 13, 2024

Choose a reason for hiding this comment

JorgeMucientes commented Feb 13, 2024

peril-woocommerce bot commented Feb 13, 2024

codecov-commenter commented Feb 13, 2024 • edited Loading

Codecov Report

0nko commented Feb 13, 2024

wpmobilebot commented Feb 9, 2024 •

edited

Loading

codecov-commenter commented Feb 13, 2024 •

edited

Loading