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

Filter out closed AdSense accounts based on new API capabilities #5050

Closed
felixarntz opened this issue Apr 7, 2022 · 10 comments
Closed

Filter out closed AdSense accounts based on new API capabilities #5050

felixarntz opened this issue Apr 7, 2022 · 10 comments
Labels
Good First Issue Good first issue for new engineers Module: AdSense Google AdSense module related issues P0 High priority Type: Bug Something isn't working

Comments

@felixarntz
Copy link
Member

felixarntz commented Apr 7, 2022

Based on the recently released new AdSense API capabilities and the corresponding enhancements to the design doc, we need to ensure closed AdSense accounts are ignored by Site Kit. For example, if a user has only a single closed account, they will need to create a new AdSense account, which is the same as if they had no account at all.


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

Acceptance criteria

  • The AdSense GET:accounts data point should filter out any accounts from the API response that have a state: CLOSED, as they can be ignored from our perspective.

Implementation Brief

  • Using includes/Modules/AdSense.php,
    • Add a new public static function is_account_not_closed which should have the account array as parameter. The function should return a boolean, whether account.state is different from CLOSED.
    • Within GET:accounts in parse_data_response, instead of returning the result of $response->getAccounts (with the filtered account IDs), store the accounts in a variable ($accounts) and then use array_filter to filter the array, passing the array ($accounts) as first parameter and the function filter_account_with_state_closed as second parameter. Return the updated array.

Test Coverage

  • No new tests to be added.

QA Brief

  • Use a Google account where you have a closed AdSense account.
  • Start the AdSense module setup and verify that this account does not show up or that there are no errors due to this account being chosen.
  • Basically, if that closed AdSense account is the only one you have, you should land in the UI to create a new AdSense account.
  • This should be tested both with and without the adsenseSetupV2 flag enabled.
  • This isn't exactly possible to set up in a reasonable timeframe, but @felixarntz has such an account, so he is going to QA this issue.

Changelog entry

  • Ensure closed AdSense accounts are not considered for the AdSense account to use with the module.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature Module: AdSense Google AdSense module related issues labels Apr 7, 2022
@felixarntz felixarntz self-assigned this Apr 7, 2022
@felixarntz felixarntz added the Good First Issue Good first issue for new engineers label Apr 7, 2022
@felixarntz felixarntz removed their assignment Apr 7, 2022
@asvinb asvinb assigned asvinb and unassigned asvinb Apr 8, 2022
@sancodes
Copy link
Contributor

sancodes commented Apr 9, 2022

If this is open I would like to help. However, it'll be my first time tackling php codebase, but I can try my best. Please let me know

@felixarntz felixarntz self-assigned this Apr 10, 2022
@felixarntz
Copy link
Member Author

@asvinb Just one thing regarding the IB: The filter_account_with_state_closed doesn't really sound accurate. Since it's a boolean check for whether the account is not closed, how about we call it is_account_open or is_account_not_closed or something like that? Other than that the IB looks good.

@felixarntz
Copy link
Member Author

@sancodes Absolutely, your contribution would be welcome! We need to complete this issue within the next week to get it into the upcoming Site Kit release, so if you could work on a pull request in the next few days that would be great. Before starting the pull request, please review the "Implementation Brief" in the issue description. It describes the technical approach to take, so you can follow that for the PR. It will probably also help you especially as you're new to the PHP codebase.

@felixarntz felixarntz assigned asvinb and unassigned felixarntz Apr 10, 2022
@sancodes
Copy link
Contributor

@felixarntz will do thanks.

@asvinb
Copy link
Collaborator

asvinb commented Apr 11, 2022

@felixarntz IB has been updated as per your feedback. Can you take another look? Thanks!

@asvinb asvinb assigned felixarntz and unassigned asvinb Apr 11, 2022
@felixarntz
Copy link
Member Author

@asvinb IB ✅

@sancodes Awesome, thank you! This is ready to be worked on now.

@sancodes
Copy link
Contributor

@felixarntz will start on it thank you.

@felixarntz
Copy link
Member Author

QA ✅

  • Before this fix, starting the AdSense setup flow causes an error because it chooses my closed AdSense account.
    • The account is returned by our API endpoint.
    • The UI keeps loading due to the unexpected error.
  • After this fix, starting the AdSense setup flow works correctly since my closed AdSense account is now ignored.
    • The account is no longer returned by our API endpoint.
    • The UI now shows me the "Create account" step (since I have no other account than the closed one).
  • This behaved the same regardless of the adsenseSetupV2 flag.

Screenshots

Before
Screen Shot 2022-04-15 at 10 46 52 AM

After
Screen Shot 2022-04-15 at 10 47 15 AM

@felixarntz felixarntz removed their assignment Apr 15, 2022
@wpdarren
Copy link
Collaborator

@felixarntz I am creating the test plan for AdSense Setup Improvement and wondering if this could now be tested by us since we have the tester plugin update. What are your thoughts? If it can, would it be possible for you to write up a new QAB, and I can make sure that this gets included. 🤔

@felixarntz
Copy link
Member Author

@wpdarren Since this is something which happens on the server-side, it wouldn't really be possible to mock this via the tester plugin.

I think this piece is simple enough so that we don't need to cover this as part of the bug bash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers Module: AdSense Google AdSense module related issues P0 High priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants