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

RRM publications list not filtering correctly #9247

Closed
8 tasks done
nfmohit opened this issue Aug 27, 2024 · 6 comments
Closed
8 tasks done

RRM publications list not filtering correctly #9247

nfmohit opened this issue Aug 27, 2024 · 6 comments
Labels
Module: RRM Reader Revenue Manager module related issues P0 High priority Team M Issues for Squad 2 Type: Bug Something isn't working

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Aug 27, 2024

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 Reader Revenue Manager GET:publications endpoint should correctly filter publications relevant to the current site only, based on the criteria mentioned in the ACs of #8791, also quoted below:
  • 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

  • In includes/Modules/Reader_Revenue_Manager.php:
    • Update the get_publication_filter method:
      • In the URL::permute_site_url method invocation, instead of passing the raw URL, pass the actual URL which includes the URL schema.
  • The above has already been covered in POC Fix publications filtering #9248. Feel free to review and merge if it looks appropriate.

Test Coverage

  • In tests/phpunit/integration/Modules/Reader_Revenue_ManagerTest.php:
    • Update the test_get_publications test case:
      • Verify that the filter query parameter is set appropriately in the request URL.
        • Test in cases of both domain and URL properties for the Search Console property ID.

QA Brief

  • Refer the Steps to reproduce section.
  • There should be no publication in the dropdown that doesn't belong to the current test site.

Changelog entry

  • Ensure the list of publications in the Reader Revenue Manager setup and settings screens only shows publications relevant to the current site.
@nfmohit nfmohit self-assigned this Aug 27, 2024
@nfmohit nfmohit added Type: Bug Something isn't working P0 High priority Team M Issues for Squad 2 Module: RRM Reader Revenue Manager module related issues labels Aug 27, 2024
@nfmohit nfmohit mentioned this issue Aug 27, 2024
18 tasks
@nfmohit nfmohit removed their assignment Aug 28, 2024
@kuasha420 kuasha420 self-assigned this Aug 28, 2024
@kuasha420
Copy link
Contributor

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 kuasha420 assigned nfmohit and unassigned kuasha420 Aug 28, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Aug 30, 2024

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!

@nfmohit nfmohit assigned kuasha420 and unassigned nfmohit Aug 30, 2024
@google google deleted a comment from nfmohit Sep 1, 2024
@kuasha420
Copy link
Contributor

Thanks @nfmohit !

IB ✅

@kuasha420 kuasha420 removed their assignment Sep 1, 2024
@ankitrox ankitrox assigned ankitrox and unassigned ankitrox Sep 2, 2024
@nfmohit nfmohit assigned nfmohit and ankitrox and unassigned nfmohit Sep 3, 2024
@ankitrox ankitrox assigned nfmohit and unassigned ankitrox Sep 4, 2024
@nfmohit nfmohit removed their assignment Sep 4, 2024
@techanvil techanvil self-assigned this Sep 4, 2024
techanvil added a commit that referenced this issue Sep 4, 2024
@techanvil techanvil removed their assignment Sep 4, 2024
@kelvinballoo
Copy link
Collaborator

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
Copy link
Collaborator Author

nfmohit commented Sep 5, 2024

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!

@ankitrox ankitrox removed their assignment Sep 5, 2024
@kelvinballoo
Copy link
Collaborator

kelvinballoo commented Sep 5, 2024

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.

  • When there is a new site with no publication, the module will prompt for creating publication. It won't pull all the publications under that accounted. And once publication was created, the dashboard will reload and prompt to complete setup. ✅

    Screenshot 2024-09-05 at 11 22 45
  • Going back to the setup and clicking edit will load the only one Publication that was linked to this URL, and not all publications linked to the account. ✅

    Screenshot 2024-09-04 at 19 10 08
  • Created a 2nd publication and when disconnected and reconnected the RRM module, both publications would appear as expected. ✅

    Screenshot 2024-09-05 at 11 48 59

@kelvinballoo kelvinballoo removed their assignment Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: RRM Reader Revenue Manager module related issues P0 High priority Team M Issues for Squad 2 Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants