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

API PULL - Validate MC connection in notifications #2336

Conversation

puntope
Copy link
Contributor

@puntope puntope commented Mar 26, 2024

Changes proposed in this Pull Request:

Follow up for #2326 (comment)

This PR introduces a check for MC Connection in the NotificationService class before the notify() is sent.

Detailed test instructions:

  1. Enable Notifications with the woocommerce_gla_notifications_enabled filter
  2. With a connected MC - Go to the Connection test
  3. Try the Notify function and see it worked
  4. Disconnect MC
  5. Try the Notify function
  6. See it failed
  7. Go to the logs and see the entry generated by the failure.

@github-actions github-actions bot added changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement. labels Mar 26, 2024
@puntope puntope self-assigned this Mar 26, 2024
@puntope puntope marked this pull request as ready for review March 26, 2024 16:00
@puntope puntope requested a review from a team March 26, 2024 16:00
@puntope
Copy link
Contributor Author

puntope commented Mar 26, 2024

UNit test not running due a problem with the API for getting the plugin versions. Seems not related to this PR.

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 64.2%. Comparing base (8ceb779) to head (c9e9bc7).
Report is 53 commits behind head on feature/google-api-project.

Additional details and impacted files

Impacted file tree graph

@@                      Coverage Diff                       @@
##             feature/google-api-project   #2336     +/-   ##
==============================================================
+ Coverage                          64.1%   64.2%   +0.2%     
- Complexity                         4450    4478     +28     
==============================================================
  Files                               472     471      -1     
  Lines                             18775   18853     +78     
==============================================================
+ Hits                              12031   12110     +79     
+ Misses                             6744    6743      -1     
Flag Coverage Δ
php-unit-tests 64.2% <70.0%> (+0.2%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/API/WP/NotificationsService.php 95.6% <100.0%> (+0.4%) ⬆️
src/Jobs/Notifications/AbstractNotificationJob.php 100.0% <ø> (ø)
src/Jobs/Notifications/CouponNotificationJob.php 100.0% <100.0%> (ø)
src/Jobs/Notifications/ProductNotificationJob.php 100.0% <100.0%> (ø)
...ernal/DependencyManagement/CoreServiceProvider.php 0.0% <0.0%> (ø)
src/ConnectionTest.php 0.0% <0.0%> (ø)

... and 5 files with indirect coverage changes

Copy link
Contributor

@jorgemd24 jorgemd24 left a comment

Choose a reason for hiding this comment

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

Thanks @puntope for adding this! It works well 👍

Do you want to merge this to feature/google-api-project or add/grant-access-to-google-wpcom-app-in-setting-page, mainly because the other changes that you did are merged in grant-access-to-google-wpcom-app-in-setting-page

@puntope puntope changed the base branch from feature/google-api-project to add/grant-access-to-google-wpcom-app-in-setting-page March 27, 2024 16:45
@puntope puntope changed the base branch from add/grant-access-to-google-wpcom-app-in-setting-page to feature/google-api-project March 27, 2024 16:45
@puntope
Copy link
Contributor Author

puntope commented Mar 27, 2024

mainly because the other changes that you did are merged in grant-access-to-google-wpcom-app-in-setting-page

I did it in the main PR because I'm not using any function or service from the grant-access-to-google-wpcom-app-in-setting-page PR.

Do you think I should merge it in grant-access-to-google-wpcom-app-in-setting-page PR. instead?

@puntope puntope changed the title Validate MC connection in notifications API PULL - Validate MC connection in notifications Mar 27, 2024
@jorgemd24
Copy link
Contributor

Hi @puntope, thanks for the clarification, actually I think it is better to merge it to feature/google-api-project so it is easier to review #2311

@puntope puntope changed the base branch from feature/google-api-project to add/grant-access-to-google-wpcom-app-in-setting-page March 28, 2024 10:29
…nto add/validate-mc-connection-in-notifications
@puntope puntope changed the base branch from add/grant-access-to-google-wpcom-app-in-setting-page to feature/google-api-project March 28, 2024 10:33
… add/validate-mc-connection-in-notifications
@puntope puntope changed the base branch from feature/google-api-project to develop April 2, 2024 10:00
@puntope puntope changed the base branch from develop to feature/google-api-project April 2, 2024 10:00
@puntope puntope merged commit 7d81fe2 into feature/google-api-project Apr 2, 2024
12 checks passed
@puntope puntope deleted the add/validate-mc-connection-in-notifications branch April 2, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants