google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 279 forks source link

RRM publications list not filtering correctly #9247

Closed nfmohit closed 1 week ago

nfmohit commented 2 weeks ago

Bug Description

It appears that due to the change introduced here, the RRM GET:publications endpoint is no longer filtering the publications based on the current site URL.

Reported during bug bash here.

Steps to reproduce

  1. In Publisher Center, create multiple publications, some with your test site URL and some with another URL.
    • You also need to set up RRM and start setting up one of the prompts for Site Kit to recognize the publications.
  2. Turn on the rrmModule feature flag from the tester plugin.
  3. Set up Site Kit with the same Google account that you used to set up publications in the publisher center.
  4. Go to Site Kit Settings -> Connect More Services. Click on "Set up Reader Revenue Manger".
  5. In the publication selection, observe that the list includes other publications that are not linked to your test site. It should only include publications that match your test site's URL.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The current Search Console property ID should be used, with the relevant permutations (http and https for URLs, www and non-www in any case).

    • The Core\Util\URL::permute_site_url() & Core\Util\URL::permute_site_hosts() methods should be used to include permutations for Punycode and Unicode variations.
  • If it is a domain property (sc-domain:…), the filter parameter should use permutations of the domain following sc-domain: and be either:

    • domain = "example.com" OR
    • domain = "www.example.com”
  • If it is a URL property (http://… or https://…), the filter parameter should use permutations of the full URL and be either:

    • site_url = "https://example.com/" OR
    • site_url = "http://example.com/" OR
    • site_url = "https://www.example.com/" OR
    • site_url = "http://www.example.com/"

Implementation Brief

Test Coverage

QA Brief

Changelog entry

kuasha420 commented 2 weeks ago

Hi @nfmohit, the IB and linked PR looks good. However, can we add some test coverage for this? Maybe add a test_get_data_publications or similar to test that the get_publication_filter function works correctly.

nfmohit commented 2 weeks ago

Hi @nfmohit, the IB and linked PR looks good. However, can we add some test coverage for this? Maybe add a test_get_data_publications or similar to test that the get_publication_filter function works correctly.

@kuasha420 Good call! I've updated the test coverage to include testing for the filter. I've also bumped up the estimate accordingly. Thanks!

kuasha420 commented 1 week ago

Thanks @nfmohit !

IB :white_check_mark:

kelvinballoo commented 1 week ago

QA Update ⚠️

@ankitrox , I tried to test this but it seems like it's still pulling all my Publishings when I should only have one for this site. Could you help to double check if the fixes are working as expected on your end.

Screenshot 2024-09-04 at 17 40 26
nfmohit commented 1 week ago

Hi @kelvinballoo. Thank you for sharing your observation.

I just tested and I can confirm that the fix is working as expected. In your screenshot, I can see that you're testing the develop branch. However, the PR was originally merged to the main branch. The changes were also merged back to the develop branch, but looking at Git history, I can see that it was potentially done "after" your testing.

Could you give this a try again now that the change exists in the develop (or main) branch?

Thank you!

kelvinballoo commented 1 week ago

QA Update ✅

Thanks @nfmohit , indeed it was due to the testing being on develop and the fix was on main.

Reviewed this on develop today after the merge and it's working as expected. Moving ticket to approval.