Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/12494 blaze webview fragment #12648

Merged
merged 14 commits into from
Sep 25, 2024

Conversation

JorgeMucientes
Copy link
Contributor

@JorgeMucientes JorgeMucientes commented Sep 20, 2024

Closes: #12494 and #12547

Description

This PR adds 3 things:

  • Creates a new campaign detail webview to handle actions over the campaign.
  • Refreshes Blaze campaign list when coming back from campaign detail webview and the campaign has been stopped/cancelled
  • Triggers Blaze native flow when user clicks on "Promote again" CTA

Testing information

  1. Log into a site with Blaze enabled
  2. Create a Blaze campaign
  3. From Blaze campaign list screen, click on the active campaign you've just created and Stop/Cancel it.
  4. Navigate back and check how the campaign list is updated with the most recent status updates
  5. Click again on any campaign from the list.
  6. From the campaign detail webview click on "Promote again" CTA
  7. See how the native campaign flow is triggered for the same product.

The tests that have been performed

  • Tested Blaze campaign list refreshes correctly
  • Tested promote again CTA triggers Blaze campaign creation.

Images/gif

demo.mp4
  • 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.

@JorgeMucientes JorgeMucientes added this to the 20.6 milestone Sep 20, 2024
@JorgeMucientes JorgeMucientes added the feature: blaze Related to the Blaze project label Sep 20, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Sep 20, 2024

3 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ Class ViewState is missing tests, but unit-tests-exemption label was set to ignore this.
⚠️ This PR is assigned to the milestone 20.6. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 20, 2024

📲 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
FlavorJalapeno
Build TypeDebug
Commitfbb92ac
Direct Downloadwoocommerce-wear-prototype-build-pr12648-fbb92ac.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 20, 2024

📲 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
FlavorJalapeno
Build TypeDebug
Commitfbb92ac
Direct Downloadwoocommerce-prototype-build-pr12648-fbb92ac.apk

@JorgeMucientes JorgeMucientes added the type: task An internally driven task. label Sep 23, 2024
@JorgeMucientes JorgeMucientes marked this pull request as ready for review September 23, 2024 10:33
@JorgeMucientes
Copy link
Contributor Author

While I added unit test to the new viewModel, the lint checks still fail stating the following:

[2024-09-23T10:35:22Z] Errors:
--
[2024-09-23T10:35:22Z] - [ ] Please add tests for class `ViewState` (or add `unit-tests-exemption` label to ignore this).

That doesn't make sense, so I'm adding the unit-tests exemption tag to the PR

@hichamboushaba
Copy link
Member

That doesn't make sense, so I'm adding the unit-tests exemption tag to the PR

Yes, that's what I would do to, adding unit tests for a plain class like the ViewState shouldn't be needed.

@hichamboushaba hichamboushaba self-assigned this Sep 25, 2024
Copy link
Member

@hichamboushaba hichamboushaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @JorgeMucientes, it works well, I left a minor suggestion, but it's not a blocker.

Comment on lines 31 to 32
blazeUrlsHelper.buildCampaignsListUrl() == url -> onDismiss()
url.contains(blazeUrlsHelper.getCampaignStopUrlPath(navArgs.campaignId)) -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor, I would suggest caching the results of blazeUrlsHelper.buildCampaignsListUrl() and blazeUrlsHelper.getCampaignStopUrlPath(navArgs.campaignId) and use the cached values instead of calling the functions here again and again, the onUrlLoaded will be called quite often.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Applied!

@JorgeMucientes JorgeMucientes merged commit ee71f87 into trunk Sep 25, 2024
14 of 15 checks passed
@JorgeMucientes JorgeMucientes deleted the issue/12494-blaze-webview-fragment branch September 25, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: blaze Related to the Blaze project type: task An internally driven task. unit-tests-exemption
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refresh Blaze campaign list when a campaign is cancelled
4 participants