Fix navigation issue when triggering blaze from product detail

[JorgeMucientes]

Closes: #12762

Description

This was way trickier than I expected considering how minor/simple it look the navigation bug.

When triggering Blaze creation flow from product detail, the intro screen is always shown, event when the user has already several campaigns created. This PR fixes that.

I'm not super happy with the final solution so I'm open to hearing alternatives suggestions, but after trying different approaches this was the only one I found that worked for all cases.

I'll try to explain the reason for the bug and the proposed solution.

Cause of the bug
With the current implementation from trunk we use the property graph of the NavController; this property points to the main graph attached to the current NavHost, which is nav_graph_main (when the app is in single pane mode, more on this later), so when we call findNode on it, we retrieve the nested graph attached to it, and not the one we want to modify.

Proposed solution

• Retrieve the parent nav_graph instead and override the start destination there.

Testing information

1. From a Blaze eligible site with multiple campaigns created
2. Open product detail
3. Tap on Promote with Blaze
4. See how intro screen is never shown

Repeat steps 1-3 on a site eligible for Blaze but without any Blazd campaigns. Verify that intro screen is displayed.

The tests that have been performed

Smoked tested triggering Blaze campaign creation flow from all possible access points.
I also verified that the events 🔵 Tracked: blaze_entry_point_tapped, Properties: {"source":"intro_view", are still tracked correctly for each of the cases.

• I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

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 big (tablet) and small (phone) in case of UI changes, and no regressions are added.

[dangermattic]

	1 Warning
⚠️	This PR is assigned to the milestone 20.8. The due date for this milestone has already passed. Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

[wpmobilebot]

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

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	babcece
	Direct Download	woocommerce-wear-prototype-build-pr12763-babcece.apk

[JorgeMucientes]

Unrelated to the fix, but this was not needed, as this is already handled inside navigateToBlazeGraph function.

[wpmobilebot]

📲 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
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	babcece
	Direct Download	woocommerce-prototype-build-pr12763-babcece.apk

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 40.86%. Comparing base (4592e2d) to head (babcece).
Report is 7 commits behind head on trunk.

Files with missing lines	Patch %	Lines
.../blaze/creation/BlazeCampaignCreationDispatcher.kt	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##              trunk   #12763   +/-   ##
=========================================
  Coverage     40.86%   40.86%           
  Complexity     5751     5751           
=========================================
  Files          1236     1236           
  Lines         69726    69725    -1     
  Branches       9669     9669           
=========================================
  Hits          28496    28496           
+ Misses        38602    38601    -1     
  Partials       2628     2628           

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

[hichamboushaba]

Thanks @JorgeMucientes for the investigation and the fix.

but that override is lost along the way because NavController.navigate() ends up calling val node = findDestination(destId) which essentially retrieves the nav_graph_blaze_campaign_creation as it is defined in the XML code (without the start destination override).

I tried to understand why this fails for product details and not for the other places, because since it works in other places, the above explanation wouldn't explain it.

After checking the implementation of findDestination, I was able to figure out what causes the issue, for our implementation, we use the property graph of the NavController (my mistake actually 🤦‍♂️), this property points to the main graph attached to the current NavHost, which is nav_graph_main (when the app is in single pane mode, more on this later), so when we call findNode on it, we retrieve the nested graph attached to it, and not the one we want to modify.

Regarding the mention about single pane mode, the above explanation also explains why the issue is not reproducible on tablets where multiple panes are used, because then the NavHost we use uses nav_graph_products directly instead of nav_graph_main.

With the above, the fix would be easy, we just need to retrieve the current navgraph where the current destination sits, the below patch fixes the issue for me:

Index: WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/BlazeCampaignCreationDispatcher.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/BlazeCampaignCreationDispatcher.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/BlazeCampaignCreationDispatcher.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/BlazeCampaignCreationDispatcher.kt	(revision 2cbe776d5a120a79298cd1aebe59b7bbb3ae06d0)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/BlazeCampaignCreationDispatcher.kt	(date 1728064643670)
@@ -147,9 +147,6 @@
     }
 
     private fun BaseFragment.showProductSelector() {
-        val navGraph = findNavController().graph.findNode(R.id.nav_graph_blaze_campaign_creation) as NavGraph
-        navGraph.setStartDestination(R.id.nav_graph_product_selector)
-
         findNavController().navigateToBlazeGraph(
             startDestination = R.id.nav_graph_product_selector,
             bundle = ProductSelectorFragmentArgs(
@@ -166,7 +163,10 @@
         startDestination: Int,
         bundle: android.os.Bundle? = null,
     ) {
-        val navGraph = graph.findNode(R.id.nav_graph_blaze_campaign_creation) as NavGraph
+        // Retrieve the parent navgraph.
+        // currentDestination should never be null, but we're being cautious here by falling back to the main graph.
+        val parentGraph = currentDestination?.parent ?: graph
+        val navGraph = parentGraph.findNode(R.id.nav_graph_blaze_campaign_creation) as NavGraph
         navGraph.setStartDestination(startDestination)
 
         navigateSafely(

Please let me know what you think.

[JorgeMucientes]

Hey @hichamboushaba sorry for the late reply. Getting back to this.

First of all thanks for taking the time to look into it and provide a much better/simpler solution.

I tried to understand why this fails for product details and not for the other places, because since it works in other places, the above explanation wouldn't explain it.

The reason it worked for the other two destinations is because:

• Triggering the product picker graph would load a whole new nested graph. We didn't need to override start destination. So most of the CTAs for Blaze in the app where not impacted by the bug.
• Same goes for intro screen. Since it by default the start destination of blaze_graph, all worked as expected.

It was only when we tried navigating directly to BlazeCampaingPreview (which is nested inside blaze_graph) that the start destination override was lost.

Anyway retrieving the current parent graph works like a charm. I just wished I realised about that sooner instead of applying all the prior changes 😅

[hichamboushaba]

I tried to understand why this fails for product details and not for the other places, because since it works in other places, the above explanation wouldn't explain it.
The reason it worked for the other two destinations is because:

Triggering the product picker graph would load a whole new nested graph. We didn't need to override start destination. So most of the CTAs for Blaze in the app where not impacted by the bug.
Same goes for intro screen. Since it by default the start destination of blaze_graph, all worked as expected.
It was only when we tried navigating directly to BlazeCampaingPreview (which is nested inside blaze_graph) that the start destination override was lost.

Thanks @JorgeMucientes, that's not what I meant exactly, I meant: "why we correctly skip the intro when the Blaze creation is launched from other places (like from the dashboard)", this pushed me to question the explanation given above in the PR description 🙂, because we have the default startDestination set to the the Intro in the graph, and yet the override works when launching from the dashboard, but doesn't work when launching from the product details.

Anyway, this is now clear as to why it happened, as the override was done the wrong graph 🤷.

[hichamboushaba]

Looks good 👍

Can we update the PR description to clarify that the cause of the issue differs from what is currently stated, and include a link to the pertinent discussion below?