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

Clarify requirements for displaying Free Ads Credits #2599

Closed
joemcgill opened this issue Sep 10, 2024 · 7 comments
Closed

Clarify requirements for displaying Free Ads Credits #2599

joemcgill opened this issue Sep 10, 2024 · 7 comments

Comments

@joemcgill
Copy link
Collaborator

While working on #2558, it was discovered that the useFreeAdCredit() hook, which is intended to determine whether the Google Ads account is eligible for the $500 free ads credit, was returning false in situations where the credit was expected to be visible (see related conversation).

In order to ensure this hook is working as expected, we need to clarify the requirements of this hook. Right now, it works by selecting the googleAdsAccount object returned from useGoogleAdsAccount() and checking to see if the googleAdsAccount.sub_account value is true and if the googleAdsAccount.created_timestamp value is less than 60 days ago. However, in many cases where this hook is used, those values are not available in the data store because they are only returned by the API during a specific part of the setup process when creating a new Ads ID.

case 'set_id':
// Just in case, don't create another Ads ID.
if ( ! empty( $ads_id ) ) {
break;
}
$account = $this->container->get( Middleware::class )->create_ads_account();
$step['data']['sub_account'] = true;
$step['data']['created_timestamp'] = time();
break;

My understanding is that Ads credits should be shown whenever the Ads account was created by the plugin within the last 60 days (i.e. not when an existing account is connected, or when the account is older that 60 days). Are there other requirements that need to be considered before updating this hook so it can be used throughout the admin to determine when the Free Ads Credit should be shown?

@eason9487
Copy link
Member

In the v1 version, when linking to an existing account, it was not able to determine whether the account was a sub-account or not, nor to know when it was created. Ref:

  • Add created data with ads account status #329

    The data is only added when we create the account, since when we are linking we can't confirm whether it's a sub_account or not. So returning it as false might not be correct. If a user creates a sub_account and then disconnects and then relinks the same sub_account we will lose the created_timestamp and the boolean sub_account. We are accepting this as a limitation in v1.

  • * To be eligible for the free ad credit, the account must be a sub-account
    * that is newly created within the last 60 days.
    * If users disconnected the account and reconnect again,
    * it will not be seen as a newly created sub-account
    * and hence won't be eligible for the free ad credit.

Therefore, the implementation at that time could only be narrowly storing those two properties for the account newly created via this plugin. In other words, it was implemented on the basis of avoiding false positives.

The logic in useFreeAdCredit is still working today. The free credit banner was shown correctly in testing with the 90fd7e7 revision.

2024-09-11 12 52 09

@joemcgill
Copy link
Collaborator Author

Thanks, @eason9487. The current logic does work when creating a new account, but only when you're still in the state where that account was created. If the app is refreshed, or if the hook is used in a different context (like when wanting to display these free ads credit banners on the plugin dashboard #2538, pending designs) then this data can no longer be used.

After talking with @fblascogarma the guidance from Google was that there too many internal factors for us to implement logic to only show these banners when an account is eligible and instead we should show it in nearly all cases as long as we continue including the "Terms and conditions apply" link. See this Slack convo for more context.

Next steps here would be to:

  • Remove the 60-day logic since even some accounts created less than 60 days ago are not eligable.
  • Try to limit this to only show for accounts that have been created by this plugin vs existing accounts that have been connected. @mikkamp do you know if we have a way of determining that on the API side (i.e., whether the account is a sub account)? If not, then we should essentially always show these banners and remove the hook used as a conditional.

@eason9487
Copy link
Member

The current logic does work when creating a new account, but only when you're still in the state where that account was created. If the app is refreshed, or if the hook is used in a different context (like when wanting to display these free ads credit banners on the plugin dashboard #2538, pending designs) then this data can no longer be used.

I'm not pretty sure the meaning of app is refreshed. Does it mean the plugin data is wiped out? Otherwise, if continue using the newly created and connected Ads account, the sub_account and created_timestamp data should keep existing and returning along with the wc/gla/ads/connection API.

  • 'ads/connection',
    [
    [
    'methods' => TransportMethods::READABLE,
    'callback' => $this->get_connected_ads_account_callback(),
  • protected function get_connected_ads_account_callback(): callable {
    return function () {
    return $this->account->get_connected_account();
    };
    }
  • public function get_connected_account(): array {
    $id = $this->options->get_ads_id();
    $status = [
    'id' => $id,
    'currency' => $this->options->get( OptionsInterface::ADS_ACCOUNT_CURRENCY ),
    'symbol' => html_entity_decode( get_woocommerce_currency_symbol( $this->options->get( OptionsInterface::ADS_ACCOUNT_CURRENCY ) ) ),
    'status' => $id ? 'connected' : 'disconnected',
    ];
    $incomplete = $this->state->last_incomplete_step();
    if ( ! empty( $incomplete ) ) {
    $status['status'] = 'incomplete';
    $status['step'] = $incomplete;
    }
    $status += $this->state->get_step_data( 'set_id' );

(I don't/didn't mean to have opinions on the new requirement, but only to explain the current implementation and past considerations in development.)

@mikkamp
Copy link
Contributor

mikkamp commented Sep 25, 2024

Try to limit this to only show for accounts that have been created by this plugin vs existing accounts that have been connected. @mikkamp do you know if we have a way of determining that on the API side (i.e., whether the account is a sub account)? If not, then we should essentially always show these banners and remove the hook used as a conditional.

No, we don't have a way of doing so. With an Ads account it has a link to an MCC account but this link is established whether it was initially created by one of the MCC's or not. I'm not aware of any account information which shows how the account was created.

@joemcgill
Copy link
Collaborator Author

Thanks, @mikkamp. In that case, I believe we should deprecate the useFreeAdCredit() hook and instead always show the Free Ads credit where applicable. We can include this update in the requirements for #2538, since that will be the first place where the ads credit will be used outside one of the campaign creation flows. @eason9487 does that work for you?

@eason9487
Copy link
Member

[...] @eason9487 does that work for you?

I don't have opinions on the direction of requirements. I mostly focus on the implementation and technical things, so sure, the useFreeAdCredit can be removed after confirming no other scenarios need it.

@joemcgill
Copy link
Collaborator Author

Thanks. I'm going to mark this issue as closed now, and we can discuss the implementation details in #2538.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants