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

Improve the resetPublications action #9176

Closed
10 tasks
nfmohit opened this issue Aug 8, 2024 · 6 comments
Closed
10 tasks

Improve the resetPublications action #9176

nfmohit opened this issue Aug 8, 2024 · 6 comments
Labels
Module: RRM Reader Revenue Manager module related issues P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Aug 8, 2024

Feature Description

The RRM resetPublications action currently invalidates the resolver for getPublications first before it clears the list of publications in store. As @aaemnnosttv pointed out here, this may result in a situation where the selector may run and not fetch if publications are set in state. This order should be changed.


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

Acceptance criteria

  • The Reader Revenue Manager data store's resetPublications action should clear the existing publications from the store before it invalidates the getPublications resolver.
  • The expected behaviour after the publications reset should be maintained, i.e.
    • During module setup, if there are no publications to start with and the user clicks on "Create publication", after creating the publication and the setup screen reloads, it should correctly display the second step with the "Complete setup" CTA.
    • During module setup, if there are existing publications, and the user clicks on the "Create publication" link, after creating the publication and the setup screen reloads, it should correctly display the same screen (with the publication select updated with the newly created publication).

Implementation Brief

The following implementation brief has already been covered in this PoC, which can directly be moved to CR if deemed appropriate.

  • In assets/js/modules/reader-revenue-manager/datastore/publications.js:
    • Update the resetPublications action:
      • At first, yield the RESET_PUBLICATIONS action type.
      • Afterwards, clear stored errors for the getPublications selector.
      • At last, invalidate the resolution for the getPublications resolver and return its response.
      • In short, this commit can just be undone.
  • In assets/js/modules/reader-revenue-manager/components/setup/SetupMain.js:
    • Update the publications variable so that it does not return an empty array as a fallback.
    • Update the useEffect that is responsible for setting the SHOW_PUBLICATION_CREATE key for the READER_REVENUE_MANAGER_SETUP_FORM core/forms form to ensure that it is only set if publications is not undefined (beside existing conditions).
      • This makes sure that the setup screen suddenly does not render the PublicationCreate component after publications are reset when one or more publications were already available.

Test Coverage

  • Fix any failing tests.

QA Brief

  • Using the tester plugin, turn on the rrmModule feature flag.
  • Set up Site Kit with a Google account that does not have any publications in the publisher center.
  • Set up the RRM module.
  • In the publication creation screen, click on "Create publication" CTA. You should be taken to the publisher center in a new tab.
  • Create a new publication and return to the RRM setup tab (after at least 15 seconds).
  • Verify that the setup flow reloads and goes to the second step of publication creation, with the "Complete setup" CTA displayed.
  • Complete setup.
  • From Site Kit Settings -> Connected Services, disconnect the RRM module.
  • From Site Kit Settings -> Connect More Services, re-connect the RRM module.
  • In the setup screen, under the publication select, click on the "Create publication" link. You should be taken to the publisher center in a new tab.
  • Create a new publication and return to the RRM setup tab (after at least 15 seconds).
  • Verify that the setup flow reloads and the same step is shown (with the publication select and the "Create publication" link). Verify that you see the new publication in the publication select (but do not change your publication selection).
  • Go to another tab and wait for over 15 seconds.
  • Come back to the setup tab and verify that the setup flow reloads again.
  • Change your publication selection.
  • Go to another tab and wait for over 15 seconds.
  • Come back to the setup tab and verify that the setup flow no longer reloads.

Changelog entry

  • Modify the Reader Revenue Manager's resetPublications action to clear publications before invalidating the getPublications resolver.
@nfmohit nfmohit self-assigned this Aug 8, 2024
@nfmohit nfmohit added P2 Low priority Type: Enhancement Improvement of an existing feature P1 Medium priority Module: RRM Reader Revenue Manager module related issues Team M Issues for Squad 2 and removed P2 Low priority labels Aug 8, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Aug 12, 2024

Note for AC Reviewer

I've also added an IB (including a PoC). If the IB looks good to you, please feel free to send this right to EB.

@nfmohit nfmohit removed their assignment Aug 12, 2024
@techanvil techanvil assigned techanvil and nfmohit and unassigned techanvil Aug 13, 2024
@techanvil
Copy link
Collaborator

Thanks @nfmohit. The first AC point was stating things as they are, with the invalidation occurring before the reset, and effectively needed to be reversed, which I have done. With that taken care of the AC LGTM.

On the IB front, what's there looks good, but please can you update the Test Coverage section and add an estimate?

@nfmohit
Copy link
Collaborator Author

nfmohit commented Aug 13, 2024

Thank you for updating the ACs, @techanvil . I've also updated the IB, my mistake, sorry! Back to you for a final pass.

@nfmohit nfmohit assigned techanvil and unassigned nfmohit Aug 13, 2024
@techanvil
Copy link
Collaborator

Thanks, @nfmohit!

IB ✅

@techanvil techanvil removed their assignment Aug 13, 2024
@nfmohit nfmohit self-assigned this Aug 13, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Aug 13, 2024

Note: Picked this one up because I have a PoC ready for this to go, hence, it will take less time. Thanks!

@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Aug 13, 2024
@hussain-t hussain-t self-assigned this Aug 14, 2024
hussain-t added a commit that referenced this issue Aug 14, 2024
@hussain-t hussain-t removed their assignment Aug 14, 2024
@wpdarren wpdarren self-assigned this Aug 15, 2024
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • I can confirm all of the steps in the QAB are verified as expected based on the specific scenarios.
Screenshots

image
image
image
image

@wpdarren wpdarren removed their assignment Aug 16, 2024
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 P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants