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

Implement <PublicationSelect> component #8837

Closed
15 of 16 tasks
nfmohit opened this issue Jun 8, 2024 · 9 comments
Closed
15 of 16 tasks

Implement <PublicationSelect> component #8837

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

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Jun 8, 2024

Feature Description

A <PublicationSelect> component should be added for the Reader Revenue Manager module that presents the user with a list of available publications and allows them to select one of them. The selected publication should be stored in the data store within the publicationID module setting, with its onboarding state stored within the publicationOnboardingState module setting.

Screenshot for reference

image


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

Acceptance criteria

  • A PublicationSelect component should be added for the Reader Revenue Manager module that presents the user with a dropdown of all available publications, according to the Figma designs.
  • Its label should be "Publication".
  • Upon selecting a publication from the dropdown, data store values of three module settings should be updated:
    • publicationID: Should contain the publication ID of the selected publication.
    • publicationOnboardingState: Should contain the onboarding state of the selected publication.
    • publicationOnboardingStateLastSyncedAtMs: Should contain the current time, in the form of the number of milliseconds elapsed since the epoch.

Implementation Brief

  • The analytics-4 ProperySelect component can be used as a reference point here.
  • Create a new component in assets/js/module/reader-revenue-manager/components/common/PublicationSelect.js. It should accept the following props.
    • isDisabled - Flag whether Publication select should be in disabled/enable state.
    • classNames - Class names for Select component from googlesitekit-components.
    • onChange - Function to call when value in dropdown changes.
    • hasModuleAccess - Flag to tell if the user has current module access.
  • Use the getPublications selector implemented in Implement RRM getPublications() selector #8794 to get the list of publications. getPublications selector will only be called when hasModuleAccess is not false and isDisabled is not falsy.
      hasModuleAccess !== false && ! isDisabled
    • Add a new function isValidPublicationID in assets/js/modules/analytics-4/utils/validation.js which should check if publication ID is valid. A publication ID should be string of only alphabets and digits. This should be similar to isValidMeasurementID function here.
    • Add the onPublicationChange using useCallback which would be passed to onEnhancedChange prop of Select component from googlesitekit-components.
    • onPublicationChange should set the publicationID, publicationOnboardingState using set method from the create-settings-store i.e. setPublicationID and setPublicationOnboardingState. These values should be received from getPublications selector.
    • Also set publicationOnboardingStateLastSyncedAtMs using same method, it should contain the current time, in the form of the number of milliseconds elapsed since the epoch.
    • Pass the label as Publication to Select component.
    • Render a Select component with no publications (other than if one is already selected) if hasModuleAccess is explicitly false, similar to the Analytics PropertySelect component.
    • We should display a loading ProgressBar component if the publications are loading. Take the reference of PropertySelect component here which does the same.
      • We can use the isResolving selector to check if the resolver for getPublications is still running.

Test Coverage

  • Add tests for the component with different props and combinations.

QA Brief

Note 1: Ensure that you have publications setup in publisher center. Refer QAB of this issue on how to setup publications. Also, there should be atleast two publications setup for testing this.

Note 2: Ensure that "Custom Site URL" is NOT set in "Tester Settings" (Site Kit by Google Tester plugin).

  1. Go to Site Kit > Settings > Connecte More Services > Reader Revenue Manager, click Set up Reader Revenue Manager CTA.

  2. This will take you to the setup screen where initially there must be a loading progress bar till publications are getting fetched.

  3. Once publications are fetched, select component will be visible.

  4. Run the following command in browser console.

  googlesitekit.data.select('modules/reader-revenue-manager').getSettings();

which should produce the following output.

    {
      "publicationID": "",
      "publicationOnboardingState": "",
      "publicationOnboardingStateLastSyncedAtMs": 0
    }
  1. Select any publication from the list and run the same command. This time there must be publicationID and publicationOnboardingState set in the response.

  2. Repeat step 5 by selecting other publications, note that every time the publication ID needs to be different.

Changelog entry

  • Provide the Settings UI to allow a user to select a publication in the Reader Revenue Manager module.
@nfmohit nfmohit self-assigned this Jun 8, 2024
@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 Jun 8, 2024
@nfmohit nfmohit removed their assignment Jun 8, 2024
@ivonac4 ivonac4 added Module: RRM Reader Revenue Manager module related issues and removed Module: RRM Reader Revenue Manager module related issues labels Jun 11, 2024
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jun 14, 2024
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jun 18, 2024
@ankitrox ankitrox assigned ankitrox and unassigned ankitrox Jun 25, 2024
@nfmohit nfmohit self-assigned this Jun 26, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jun 26, 2024

Thank you for the IB, @ankitrox ! Please take a look at my comments below:

  • Update the getPublications selector to also retrieve onboardingState in publication object. This should already be received from GET:publications endpoint.

Let's remove this requirement as this property should already be a part of the getPublications selector. See this comment.


I think it might be beneficial to use the Analytics PropertySelect component as a starting point. See points below:

  1. Let's mention the usage of the isDisabled and hasModuleAccess props. The getPublications selector should only be called if hasModuleAccess isn't explicitly false, and isDisabled isn't falsy, similar to the Analytics PropertySelect component.
  2. Let's add validation for the publication ID, i.e. isValidPublicationID function. This should be similar to the Analytics isValidMeasurementID function which checks that the value is a string which only contains alphabets and numbers.
  3. Let's mention that we want to render a Select component with no publications (other than if one is already selected) if hasModuleAccess is explicitly false, similar to the Analytics PropertySelect component.
  4. Let's mention that we want to display a loading progress bar when the publications are being resolved, again, similar to the Analytics PropertySelect component.

For Storybook stories, I don't think it'll be very valuable to add a Storybook story for this component alone. This will likely be included in the Storybook stories for the module setup and settings screens, so an individual story may seem redundant.


Let me know what you think and if you have any questions, thank you!

@nfmohit nfmohit assigned ankitrox and unassigned nfmohit Jun 26, 2024
@ankitrox
Copy link
Collaborator

Thanks @nfmohit for reviewing the IB.

I have updated the IB as per the suggested points. Re-assigning to you for review.

Thanks again!

@ankitrox ankitrox assigned nfmohit and unassigned ankitrox Jun 27, 2024
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Jun 27, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jun 30, 2024

Thank you for the updates, @ankitrox ! I've made a few changes to the IB to reduce the back and forth, namely:

  1. I've mentioned using Analytics PropertySelect as a reference point.
  2. I've removed the proposal to update the getPublications selector for onboarding state, as that should be no longer required.
  3. I made the criteria for isValidPublicationID a little more specific and mentioned that it should only accept alphabets and digits (no symbols).
  4. I've mentioned that we can use the isResolving selector for getPublications to determine the loading state, instead of proposing to create a new selector. Working on a new selector unless necessary also includes adding test coverage for it, thus requiring more time and effort.

Please let me know if you have any questions. IB LGTM 👍 ✅

@nfmohit nfmohit removed their assignment Jun 30, 2024
@eclarke1 eclarke1 removed the Next Up Issues to prioritize for definition label Jul 8, 2024
@ankitrox ankitrox self-assigned this Jul 11, 2024
@ankitrox
Copy link
Collaborator

ITEM 1: Copy

As you can see from the discussion on previous PR, we just added the component along with the message so that it is convenience to test. We will test it thoroughly in #8800

ITEM 2: Publication ID Missing and dropdown size

Sorry for not including this. It looks like this was missed during our previous round. I have created a new PR #9073 to address the issue. Also, addressed the width issue in same PR, although I believe that we could have kept the width as is to maintain consistency along with other select components. We can check if there is any feedback in CR for the same.

ITEM 3: Value for publicationOnboardingStateLastSyncedAtMs

This is will be handled in #8796 here . The reason, it can't be seen in action as of now is because we are yet to integrate everything in setup screen. We can test this in #8800

ITEM 4: Spacing

As per the discussion mentioned previously, we can test all the styling things in #8800

@ankitrox ankitrox removed their assignment Jul 25, 2024
@techanvil techanvil assigned techanvil and ankitrox and unassigned techanvil Jul 25, 2024
@ankitrox ankitrox assigned techanvil and unassigned ankitrox Jul 25, 2024
@techanvil techanvil assigned ankitrox and unassigned techanvil Jul 25, 2024
@techanvil
Copy link
Collaborator

Back to you for another pass, @kelvinballoo.

Please note that with regard to Item 2, the dropdown size is derived from content. As per this #9073 (comment) (which you may have already seen):

... the Select dropdown takes its width from its content, and this should be understood to be reflected in the Figma design rather than a fixed minimum width.

@kelvinballoo
Copy link
Collaborator

QA update ✅

Noted on Item 1, 3 and 4.
Tested item 2 and the dropdown is now reflecting the Publication ID accordingly, similar to the Publisher Central.

Screenshot 2024-07-26 at 11 33 52

Also tested the following:

  • New dropdown's label is "Publication".

  • Running googlesitekit.data.select('modules/reader-revenue-manager').getSettings(); will give 0 if nothing is selected at the dropdown.

    Screenshot 2024-07-26 at 11 41 01
  • If there a publication is selected, the details related to that publication will appear when running the script.

    Screenshot 2024-07-26 at 11 40 13

Moving ticket to Approval.

@techanvil
Copy link
Collaborator

Moving this back to execution to fix the RRM SetupMain story that developed an error as a result of its first PR.

@aaemnnosttv
Copy link
Collaborator

Thanks @techanvil , merged 👍

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

7 participants