-
Notifications
You must be signed in to change notification settings - Fork 297
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
Update RRM snippet placement logic #9973
Comments
@aaemnnosttv While brainstorming the ACs for this issue, I realised that I did not consider an edge-case scenario. Suppose, in a certain post, the RRM tag is not added based on the above logic. However, the user has added the inline button block (#9963). Since we're not adding the snippet, the button will not work. Do you have a suggestion on how we can address this edge-case? Thank you! |
Update: Upon further discussion regarding this internally with @aaemnnosttv, we've decided that we are not going to alter the snippet placement logic based on the addition of the inline button blocks. This edge case will be handled by the blocks themselves. I'll draft the ACs and move this forward accordingly. |
Thank you for drafting the ACs, @hussain-t! One general feedback regarding the overall ACs is that we should consider using less technical language and more UI-oriented instructions, as ideally, we should aim for the ACs to be conveniently readable, such as the QAB. For example:
I understand the UI for many of the above has not been implemented yet or is still being defined; however, you should see them quite well described in the design document and the Figma designs. Apart from the above:
We don't need to specify the above, as all three settings have default values, so it's unlikely they will be unavailable. Please let me know if you have any questions or thoughts; thank you! |
Thanks, @nfmohit. I've addressed your feedback by updating the AC to use more UI-oriented language from the design doc. LMK if it looks good. |
Thank you @hussain-t! I've made some further changes to the AC, including removing any mention of the term-level settings as we've decided to postpone that, and some minor cosmetic changes. Moving this forward, thanks! |
Hey @nfmohit, thanks for drafting this IB. It's most of the way there, I just have a few, mostly minor points for your consideration: Point 1:
This seems slightly at odds with the AC/design doc requirements: the above logic suggests that the snippet mode From "snippet placement" in the design doc (which the AC is consistent with):
Point 2:
A minor point, but I would suggest Point 3:
Looks like you mean of type Point 4: Lastly, I think we need to consider the implication of #10228, which is that we'll presumably need to strip the publication ID from the product ID before replacing
|
@techanvil Thank you for the kind IBR. I have updated the IB and addressed all four very valid and nicely captured points. Back to you for another pass, thank you! |
Thanks @nfmohit! That's looking good. One last point:
It's worth bearing in mind that the post product ID may have a value of |
@techanvil Excellent point, I've updated the IB, thank you! |
That's great, thanks @nfmohit! IB ✅ |
Feature Description
The Reader Revenue Manager snippet placement logic should be updated to enable conditional and context-aware snippet placement, as follows:
googlesitekit_rrm_{Publication ID}:productID
post meta setting will be checked.none
, no snippet will be added to the post.googlesitekit_rrm_{Publication ID}:productID
term meta setting (if possible, a meta query will be used to query all terms with the meta set). The value for the first found match will be checked.none
, no snippet will be added to the post.snippetMode
module setting will be checked.per_post
, no snippet will be added.post_type
, it will be checked if the post type of the current post belongs to thepostTypes
array module setting.productID
module setting as the product ID.productID
module setting as the product ID.snippetMode
module setting will be checked.sitewide
, the snippet will be added with the value of theproductID
module setting as the product ID.If none of the above conditions are met, no snippet will be added.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
none
), then no snippet should appear on that post.Implementation Brief
Conditionally outputting the snippet
includes/Modules/Reader_Revenue_Manager/Tag_Guard.php
:$post_product_id
of string type.$settings
as an instance ofGoogle\Site_Kit\Core\Modules\Module_Settings
and$post_product_id
as a string.$settings
passed to it.$post_product_id
to the$this->post_product_id
.can_activate
method should returntrue
if the following conditions are met:rrmModuleV2
feature flag is not enabled:rrmModuleV2
feature flag is enabled:is_singular()
):$this->post_product_id
is notnone
.$this->post_product_id
is not an empty string, or, If in case it is an empty string:$settings[snippetMode]
) is notper_post
.post_type
, the current post type (get_post_type()
) belongs to the RRM CTA post types. The RRM CTA post types should be obtained using thegooglesitekit_reader_revenue_manager_cta_post_types
filter, with a default value of$settings[postTypes]
.sitewide
.Embedding the Product ID
includes/Modules/Reader_Revenue_Manager/Web_Tag.php
:$product_id
of a string type.set_product_id
$product_id
.$product_id
to the$this->product_id
.enqueue_swg_script
method:$subscription
array, replaceopenaccess
with the product ID. The product ID should be obtained using a newgooglesitekit_reader_revenue_manager_product_id
filter, with a default value of$this->product_id
.wp_enqueue_script()
method instead of depending on whether it is on a single post.Update
Post_Product_ID
includes/Modules/Reader_Revenue_Manager/Post_Product_ID.php
:$settings
that would be an instance ofGoogle\Site_Kit\Modules\Reader_Revenue_Manager\Settings
.$publication_id
, accept$settings
(which would be an instance ofGoogle\Site_Kit\Core\Modules\Module_Settings
). Assign$settings
to$this->settings
.get_meta_key
method to obtain the required publication ID from$this->settings
, i.e.$this->settings->get()['publicationID']
.Connecting it all together
includes/Modules/Reader_Revenue_Manager.php
:$post_product_id
class property, which will hold an instance ofGoogle\Site_Kit\Modules\Reader_Revenue_Manager\Post_Product_ID
.Post_Product_ID
to$this->post_product_id
, passing a new instance ofPost_Meta
and the module settings ($this->get_settings()
) to it.register
method:Post_Product_ID
as we're now doing it in the constructor. Instead, simply call$this->post_product_id->register()
.register_tag
method:rrmModuleV2
feature flag is enabled and it is a singular post, call$this->post_product_id->get()
to obtain the post meta value, passing the ID of the current post (get_the_ID()
) to it as the parameter.Tag_Guard
instantiation, pass the calculated post product ID as the second parameter.$tag->register()
method, determine the final product ID to be added to the snippet:openaccess
by default.rrmModuleV2
feature flag is enabled:$settings['productID']
.openaccess
, is stored in the form of{PUBLICATION_ID}:{PRODUCT_ID}
. If that is the case, the product ID should be extracted by removing the{PUBLICATION_ID}:
prefix.$tag->set_product_id()
method, passing the calculated product ID to it.Test Coverage
tests/phpunit/integration/Modules/Reader_Revenue_Manager/Tag_GuardTest.php
:tests/phpunit/integration/Modules/Reader_Revenue_Manager/Web_TagTest.php
:tests/phpunit/integration/Modules/Reader_Revenue_Manager/Post_Product_IDTest.php
:tests/phpunit/integration/Modules/Reader_Revenue_ManagerTest.php
:QA Brief
rrmModule
andrrmModuleV2
feature flags.Clarifying different steps in the ACs
Google Reader Revenue Manager snippet added by Site Kit
).isPartOfProductId
property, which should be in this format:{YOUR_PUBLICATION_ID}:{PRODUCT ID}
, unless it isopenaccess
.googlesitekit_rrm_{YOUR_PUBLICATION_ID}:productID
, e.g.googlesitekit_rrm_CAow7tS0AD:productID
. For the value, addnone
(this will prevent snippet addition),openaccess
, or any other product ID in the format of{YOUR_PUBLICATION_ID}:{ANY RANDOM PRODUCT ID}
, e.g.CAow7tS0AD:advanced
.googlesitekit.data.dispatch( 'modules/reader-revenue-manager' ).setProductID( '{YOUR_PUBLICATION_ID}:{ANY RANDOM PRODUCT ID}' )
. You can also set it toopenaccess
, which is also the default value.googlesitekit.data.dispatch( 'modules/reader-revenue-manager' ).saveSettings()
.Changelog entry
The text was updated successfully, but these errors were encountered: