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

Notify users when an Ads account is suspended #2540

Open
6 tasks
Tracked by #2460
joemcgill opened this issue Aug 16, 2024 · 8 comments · May be fixed by #2654
Open
6 tasks
Tracked by #2460

Notify users when an Ads account is suspended #2540

joemcgill opened this issue Aug 16, 2024 · 8 comments · May be fixed by #2654
Assignees
Labels
status: on hold The issue is currently not prioritized.

Comments

@joemcgill
Copy link
Collaborator

joemcgill commented Aug 16, 2024

Part of #2460

As a merchant, I want to know if my Ads account has been suspended so that I can correct the issues so I can continue getting visibility of my products to drive more sales.

image

We'll also update the Paid Ads card on the dashboard to show that the account is suspended:

image

Acceptance Criteria

  • When an Ads account is marked as suspended, show a dismissible error notice on the plugin screen above the tabs
  • The notice should persist across all tabs in the plugin screens
  • The notice should stop showing for the logged in user once it's been dismissed
  • Clicking the Resolve Issues link should take the user to the [TK URL]
  • Once the account is no longer suspended, the notice should disappear
  • If the ads account is suspended. The Paid Ads card on the dashboard should include "(Suspended)" in the title of the card (see design)

Implementation Brief

Create a new component called AdsAccountSuspended that exports a <Notice status="error" isDismissible={ true }> component with the text and CTA button from the designs. The component should check for the account status returned from the ads/connection response to see if it's suspended (details to follow).

If the account is suspended, return the notice, otherwise return null. Import the component into the Dashboard, Reports, ProductFeed, and Reports pages listed in /js/src/index.js. and display the component above the <MainTabNav /> component.

To add the suspended status to the ads/connection response, we'll update the AdsAccountQuery to include the 'customer.status' field. Add the status returned from that query to the array returned by Ads::get_account_details(), which will need to be made public so it can be accessed from the Ads\AccountService::get_connected_account() callback used by the API, so suspended can be returned as the 'status' when applicable.

To avoid calling the Google Ads API on each REST API request, we'll cache the customer status in a transient for HOUR_IN_SECONDS.

Test Coverage

  • Update the PHPUnit tests
  • Update the E2E tests
@joemcgill joemcgill self-assigned this Aug 16, 2024
@joemcgill joemcgill added the needs design The issue requires design input/work from a designer. label Aug 16, 2024
@joemcgill joemcgill changed the title Display issues with Google Ads account or campaign status [⚠ Needs Design] Display issues with Google Ads account or campaign status Aug 16, 2024
@joemcgill joemcgill changed the title Display issues with Google Ads account or campaign status Notify users when an Ads account is suspended Oct 1, 2024
@joemcgill joemcgill removed the needs design The issue requires design input/work from a designer. label Oct 1, 2024
@joemcgill
Copy link
Collaborator Author

@mikkamp In order to implement this one, I think we'll need to update the ads/connection endpoint to include data queried from the CustomerStatus in the response. I don't think this has already been implemented, so wanted to double check if you have any preferences on how this was done. I'm thinking we could update the existing get_account_details() method to return the status of the customer, but am not confident about what the return values will be since I don't have a suspended account to test against.

@joemcgill joemcgill added the needs feedback The issue/PR needs a response from any of the parties involved in the issue. label Oct 7, 2024
@fblascogarma
Copy link
Collaborator

Hi @joemcgill and @mikkamp , starting in Google Ads API v16, which is currently used by Woo, the customer.status tells you the status of the Google Ads account. There are mainly 3 statuses: ENABLED, SUSPENDED, CANCELED. You can check all the statuses here.

For canceled accounts, you can't use the API to perform any actions.

Let me know if this helped.

@fblascogarma
Copy link
Collaborator

For canceled accounts the API will return with an error message: "The customer account can't be accessed because it is not yet enabled or has been deactivated."

Therefore, for canceled accounts you can't get any account info.

@joemcgill
Copy link
Collaborator Author

I can confirm what @fblascogarma shared.

I've got access to some cancelled accounts (I could manually set my own accounts to cancelled) and this results in a PERMISSION_DENIED response from the Google Ads API when querying for that account with the exact error message @fblascogarma shared above.

Once I'm able to confirm whether we are able to query the account info when an account is suspended, then we can fill out the technical details for this issue.

@joemcgill joemcgill added the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Oct 16, 2024
@joemcgill
Copy link
Collaborator Author

I've been able to have a test account that I control set to suspended, but even so, the AdsAccountsQuery that is run by API\Google\Ads::get_account_details() doesn't return status information that can be used in this implementation.

From what I'm seeing, all accounts are being returned with a status of 0, which seems to map to the ENUM UNSPECIFIED per the API docs.

@joemcgill
Copy link
Collaborator Author

@mikkamp Can you review the updates I've made to the implementation brief here and let me know if you'd like to see any differences?

@joemcgill
Copy link
Collaborator Author

joemcgill commented Oct 28, 2024

@mikkamp one observation I've made while starting a shallow implementation of the backend changes is that there are several places in the front end where the Ads related components are affected by whether the Ads account is connected, based on the status returned from the API (see this example).

To avoid introducing unexpected UI behavior, I think it would be safer to return the SUSPENDED status as a new field, rather than as a new value that could conditionally be returned as the status field. Given that we may want to extend this functionality in the future to also show alerts for CANCELED or CLOSED states as well, do you have a preference for how this API return value is designed?

The most straightforward for now would be to do something like:

{
    "id": $id,
    "currency": "USD",
    "symbol": "$",
    "status": "connected",
    "suspended": true
}

Then later we could rework the functionality that relies on the status field to be able to respond to additional statuses. Alternatively, we could map the CustomerStatus ENUM to a new account_status field, like this:

{
    "id": $id,
    "currency": "USD",
    "symbol": "$",
    "status": "connected",
    "account_status": "suspended"
}

@joemcgill joemcgill linked a pull request Oct 28, 2024 that will close this issue
@joemcgill joemcgill linked a pull request Oct 28, 2024 that will close this issue
@mikkamp
Copy link
Contributor

mikkamp commented Oct 29, 2024

Can you review the updates I've made to the implementation brief here and let me know if you'd like to see any differences?

The implementation brief looks good to me. One thing that we might need to work out though is cancelled accounts. I understand we don't necessarily want to query their status and show it in the banner. However the problem is that querying data from a cancelled account triggers an error. Previously the ads/connection endpoint would only get local data, now that it's doing a request to the Ads API, we need to make sure that any errors are caught and handled.

Alternatively, we could map the CustomerStatus ENUM to a new account_status field

I prefer to use the account_status field, as that will accommodate, both the actual fetched status, as well as setting an unavailable/unknown value in cases where we might need to handle an error as we are unable to fetch the status.

@joemcgill joemcgill added status: on hold The issue is currently not prioritized. and removed status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. needs feedback The issue/PR needs a response from any of the parties involved in the issue. labels Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: on hold The issue is currently not prioritized.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants