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] Enable notifications by default #2448

Merged

Conversation

puntope
Copy link
Contributor

@puntope puntope commented Jul 3, 2024

Changes proposed in this Pull Request:

This is a follow-up from this conversation p1719565119808809-slack-C02BB3F30TG

The PR enables Notifications feature by default

Detailed test instructions:

  1. Checkout this PR
  2. Complete setup
  3. Grant access to Data fetch in Settings
  4. Go to the connection test page
  5. Send a notification and verify it works
  6. Add this filter add_filter( 'woocommerce_gla_notifications_enabled', '__return_false' );
  7. Send a notification and verify it fails

Additional details:

Changelog entry

@puntope puntope self-assigned this Jul 3, 2024
@puntope puntope requested a review from a team July 3, 2024 07:07
@puntope puntope changed the base branch from develop to feature/google-api-project July 3, 2024 07:07
@github-actions github-actions bot added the changelog: tweak Small change, that isn't actually very important. label Jul 3, 2024
@puntope puntope marked this pull request as ready for review July 3, 2024 07:09
Copy link

codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 45.45455% with 6 lines in your changes missing coverage. Please review.

Project coverage is 64.4%. Comparing base (dc34f56) to head (d47c264).
Report is 1 commits behind head on feature/google-api-project.

Additional details and impacted files

Impacted file tree graph

@@                      Coverage Diff                       @@
##             feature/google-api-project   #2448     +/-   ##
==============================================================
- Coverage                          64.7%   64.4%   -0.3%     
- Complexity                         4550    4551      +1     
==============================================================
  Files                               473     795    +322     
  Lines                             17747   22796   +5049     
  Branches                              0    1219   +1219     
==============================================================
+ Hits                              11483   14688   +3205     
- Misses                             6264    7941   +1677     
- Partials                              0     167    +167     
Flag Coverage Δ
js-unit-tests 63.5% <ø> (?)
php-unit-tests 64.7% <45.5%> (-<0.1%) ⬇️

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

Files Coverage Δ
src/API/WP/NotificationsService.php 95.1% <100.0%> (ø)
src/MerchantCenter/AccountService.php 98.3% <100.0%> (+<0.1%) ⬆️
src/ConnectionTest.php 0.0% <0.0%> (ø)

... and 322 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 working on this. It works well, but I noticed that if you add the filter add_filter('woocommerce_gla_notifications_enabled', '__return_false');, the merchant won't be able to revoke access even if the status is approved. Shouldn't we allow the merchant to revoke access in the settings page regardless of the filter value? (Connection Test page is OK)

image

@puntope
Copy link
Contributor Author

puntope commented Jul 4, 2024

Hi @jorgemd24 I tweaked the code based on your comments. I also added some info in the Connection Test. Can we have a new look?

@puntope puntope requested a review from jorgemd24 July 4, 2024 06:49
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 for the adjustments. LGTM 👍

@puntope puntope merged commit 51eb4d3 into feature/google-api-project Jul 4, 2024
15 checks passed
@puntope puntope deleted the tweak/enable-notifications-default branch July 4, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: tweak Small change, that isn't actually very important.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants