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.

Sign up for GitHub

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

Jump to bottom

Integerate notification settings endpoint to disable notifications for hidden stores #14730

Merged
merged 14 commits into from
Dec 23, 2024
Merged

Integerate notification settings endpoint to disable notifications for hidden stores #14730

merged 14 commits into from
Dec 23, 2024

Conversation

itsmeichigo
Copy link
Contributor

@itsmeichigo itsmeichigo commented Dec 19, 2024 •
edited
Loading

Part of #14658

Description

This PR updates the Networking and Yosemite to support updating notification settings for a device ID given site IDs and settings. This is necessary to disable notification settings for stores hidden from the store picker, which will be handled in a future PR.

Steps to reproduce

The new endpoint hasn't been used yet so just unit tests passing is sufficient.

Testing information

I added unit tests for the Networking and Yosemite layers to ensure everything works as expected.

Screenshots

N/A

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

@itsmeichigo itsmeichigo added type: task An internally driven task. feature: notifications Related to notifications or notifs. labels Dec 19, 2024
@itsmeichigo itsmeichigo added this to the 21.4 milestone Dec 19, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 19, 2024 •
edited
Loading

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

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14730-eb3d325
Version21.2
Bundle IDcom.automattic.alpha.woocommerce
Commiteb3d325
App Center BuildWooCommerce - Prototype Builds #12265
 Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@itsmeichigo itsmeichigo marked this pull request as ready for review December 20, 2024 04:14
@itsmeichigo itsmeichigo mentioned this pull request Dec 20, 2024
Closed
@hichamboushaba hichamboushaba self-assigned this Dec 20, 2024
hichamboushaba
hichamboushaba approved these changes Dec 20, 2024
View reviewed changes
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 @itsmeichigo, code looks good 👏 .

Just sharing something for information, it seems WordPressKit already has a networking implementation of the same endpoint here, so a different approach could be to just wrap it instead of adding a new one, but I don't believe this is necessary.

@itsmeichigo
Copy link
Contributor Author

Thanks for the review @hichamboushaba!

Just sharing something for information, it seems WordPressKit already has a networking implementation of the same endpoint here, so a different approach could be to just wrap it instead of adding a new one, but I don't believe this is necessary.

I'm aware of this, but I prefer not to depend on WordPressKit in case we want to remove the dependency sometime in the future.

Base automatically changed from task/14658-store-list-update-save to trunk December 23, 2024 13:55
@itsmeichigo itsmeichigo merged commit 6019502 into trunk Dec 23, 2024
13 checks passed
@itsmeichigo itsmeichigo deleted the task/14658-integrate-notification-setting-endpoint branch December 23, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hichamboushaba hichamboushaba hichamboushaba approved these changes

@selanthiraiyan selanthiraiyan Awaiting requested review from selanthiraiyan

Assignees

@hichamboushaba hichamboushaba

Labels
feature: notifications Related to notifications or notifs. type: task An internally driven task.
Projects
None yet
Milestone
21.4
Development

Successfully merging this pull request may close these issues.
3 participants
@itsmeichigo @wpmobilebot @hichamboushaba