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

Add RRM product ID bulk edit setting #10248

Open
9 tasks
nfmohit opened this issue Feb 19, 2025 · 11 comments
Open
9 tasks

Add RRM product ID bulk edit setting #10248

nfmohit opened this issue Feb 19, 2025 · 11 comments
Assignees
Labels
Module: RRM Reader Revenue Manager module related issues Next Up Issues to prioritize for definition P0 High priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Feb 19, 2025

Feature Description

As an alternative to the formerly postponed term-level RRM snippet configuration, a bulk edit feature should be added to admin post lists that allows changing the RRM snippet configuration for a group of posts at a time.

Image

Image


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

Acceptance criteria

  • If the rrmModuleV2 feature flag is enabled, the Reader Revenue Manager module is connected, and the current user has access to the Reader Revenue Manager module:
    • In the WordPress admin post list of all post types that support setting the post-level Reader Revenue Manager snippet configuration, i.e. any public post type that supports the block editor:
      • A new column should be added to the list, labelled "Reader Revenue CTA", whose presence should be controllable using the Screen Options panel. The value should be according to the following:
        • If the post does not output the snippet, it should be "None".
        • If the post outputs the snippet:
          • Using the product ID set in Site Kit settings, it should be "Default".
          • Using the product ID set in post-level settings:
            • If openaccess, it should be "Open access".
            • Otherwise, it should be "{PRODUCT ID}", i.e. the product ID value.
      • A new custom select field should be added to the post list bulk edit form.
        • It should have a label of "Reader Revenue CTA", with the following options:
          • "— No Change —" (it should have a value of -1 to indicate an invalid option.
          • "Default" (it should have a value of an empty string).
          • "None" (it should have a value of none).
          • "Open access" (it should have a value of openaccess).
          • "{PRODUCT ID}" (this should have the product ID as value and should be repeated for each available product ID).
        • On clicking "Update", it should save the selected option as the product ID for all selected posts in bulk.

Implementation Brief

  • See this PoC as the basis for the implementation. Here are some basic directions:
    • Create Modules\Reader_Revenue_Manager\Admin_Post_List that accepts instances of Modules\Reader_Revenue_Manager\Settings and Modules\Reader_Revenue_Manager\Post_Product_ID as parameters in its constructors. These should be used to determine the available product IDs to use in the select field options, and the column content.
    • Query all public post types that have "editor" support and support REST API. In these post types:
      • Use the manage_{$post_type}_posts_columns filter to add the column.
      • Use the manage_{$post_type}_posts_custom_column action to fill column contents.
      • Use bulk_edit_custom_box hook to add the bulk edit form field.
      • Use save_post hook to save the form field value.
    • Refine the way how column content is calculated in the PoC PR to reduce duplicate code.
    • Instantiate and register the Admin_Post_List class within Modules\Reader_Revenue_Manager::register only if the rrmModuleV2 feature flag is enabled, and the module is connected.

Test Coverage

  • Add test coverage for the Admin_Post_List class.

QA Brief

Changelog entry

@nfmohit nfmohit self-assigned this Feb 19, 2025
@nfmohit nfmohit added P0 High priority Type: Enhancement Improvement of an existing feature Team M Issues for Squad 2 Module: RRM Reader Revenue Manager module related issues labels Feb 19, 2025
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Feb 19, 2025
@nfmohit nfmohit removed their assignment Feb 21, 2025
@techanvil techanvil self-assigned this Feb 21, 2025
@techanvil
Copy link
Collaborator

Hey @nfmohit, these AC are looking good.

It looks like you put together a working PoC, or was it more of a mockup? Essentially, I would like to check if the custom column can be toggled in the Screen Options panel. It's quite a substantial addition to the posts table, and it would be good to know that users can control its presence. I'd suggest this should be included in the AC.

Image

If/when the AC has been updated, let's ask Mariya to take a look to verify the copy and share any additional thoughts.

@techanvil techanvil assigned nfmohit and unassigned techanvil Feb 21, 2025
@nfmohit
Copy link
Collaborator Author

nfmohit commented Feb 21, 2025

Hi @techanvil !

Thank you for raising a very valid point. It is indeed a PoC that I intend to attach to the IB. And yes, it does let you control its presence in the Screen Options panel. I've also specified that in the AC, thank you!

Image

@nfmohit nfmohit assigned techanvil and unassigned nfmohit Feb 21, 2025
@techanvil
Copy link
Collaborator

Thanks @nfmohit, that's good to know!

One more thing that occurs to me is regarding the copy. I think having a different set of values displayed for the column vs the dropdown could be a bit confusing.

I'd suggest that, other than the "-- No Change --" value, the options in the dropdown should mirror those in the column, as these seem a bit more user-friendly and framed as a response to "Show Reader Revenue CTAs". In other words the dropdown options should be:

  • -- No Change --
  • Yes, with default selection
  • No
  • Yes, with "open access"
  • Yes, with "{PRODUCT ID}"

WDYT?

@techanvil techanvil assigned nfmohit and unassigned techanvil Feb 21, 2025
@techanvil
Copy link
Collaborator

Update: I now realise the specified dropdown options are in fact aligned with those introduced for the edit post page in #9962, so maybe they are better left as they are.

OTOH, the inconsistency I mentioned above is a bit odd so maybe we should consider aligning the copy in the column with the dropdown, or updating the dropdown both here and on the edit post page.

@nfmohit
Copy link
Collaborator Author

nfmohit commented Feb 21, 2025

I've updated the column copy to align with the select field options. Please let me know if that looks good, thanks @techanvil !

@nfmohit nfmohit assigned techanvil and unassigned nfmohit Feb 21, 2025
@techanvil
Copy link
Collaborator

Thanks @nfmohit. That looks OK... I must admit, though the previously define column values are a better fit for the column name "Show Reader Revenue CTAs".

Then again, the column name itself is a little on the long side, maybe it should be just "Reader Revenue CTA".

Rather than spend too long going back and forward on this, though, I think we should ask @marrrmarrr to take a look, as she will ultimately be signing off on the copy.

@techanvil techanvil assigned marrrmarrr and unassigned techanvil Feb 21, 2025
@nfmohit
Copy link
Collaborator Author

nfmohit commented Feb 24, 2025

Note to AC Reviewer:

In an attempt to fast-track this, I have added an IB to this with a PoC PR. If you have the capacity to review the IB, please do so. If not, please send it to IBR. If you can review the IB and it looks good, please send it to EB. Thanks!

CC: @techanvil

@marrrmarrr
Copy link
Collaborator

Since screen real estate is very scarce in this table, I would aim to go as short as possible with the copy.
So I would suggest for the column as @techanvil mentioned to have only "Reader Revenue CTA". Then we can shorten the options as well, to "Default", "opena ccess", "{PRODUCT ID}", and "Excluded" or "None".

@techanvil techanvil assigned techanvil and unassigned marrrmarrr Feb 24, 2025
@techanvil
Copy link
Collaborator

techanvil commented Feb 24, 2025

Thanks @marrrmarrr!

I've updated the AC; I wasn't sure about the open access label, which I've specified as "Open access", please let me know if you prefer it to be lower cased. I've also gone with "None" over "Excluded", again please let me know if you prefer the other option upon review.

Another detail is that I've kept the position for "None" below "Default" to align with the edit post page dropdown, but I'm wondering if it would be better placed as the last option in both cases to better promote using the CTAs. WDYT?.

One more question - do you think we should update the dropdown values on the edit post page to align with these, or leave them as they are? The dropdown label is different on that page and the more verbose options are probably helpful to keep as-is, but there's also a case to consider that they should have the same values for consistency.

@techanvil techanvil assigned marrrmarrr and unassigned techanvil and nfmohit Feb 24, 2025
@marrrmarrr
Copy link
Collaborator

@techanvil I think it's ok to slightly shorten here for the sake of space, even if it doesn't completely match the more detailed descriptions at the edit post dropdown. The key words are still there, e.g. "default" from "keep the default selection".
As far as the order in the dropdown, for consistency we should probably keep the order the same in both.

@techanvil
Copy link
Collaborator

techanvil commented Feb 24, 2025

OK great, thanks @marrrmarrr!

@nfmohit, I'm sending this one back over to you to either update PoC and/or the IB accordingly. Please note there's still an open question about the "open access" casing, as per this conversation on Slack. This might be resolved by the time you get to it, otherwise we can address it as we receive the reply.

@techanvil techanvil assigned nfmohit and unassigned marrrmarrr Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: RRM Reader Revenue Manager module related issues Next Up Issues to prioritize for definition P0 High priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

4 participants