woocommerce / google-listings-and-ads

Sync your store with Google to list products for free, run paid ads, and track performance straight from your store dashboard.
https://woo.com/products/google-listings-and-ads/
GNU General Public License v3.0
47 stars 21 forks source link

Notify users when an Ads account is suspended #2540

Open joemcgill opened 3 months ago

joemcgill commented 3 months ago

Part of https://github.com/woocommerce/google-listings-and-ads/issues/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

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

joemcgill commented 1 month ago

@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.

fblascogarma commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 4 weeks ago

@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 commented 3 weeks ago

@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"
}
mikkamp commented 3 weeks ago

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.