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

SwG module should allow admins to choose a revenue model #4230

Closed
ChrisAntaki opened this issue Oct 15, 2021 · 12 comments
Closed

SwG module should allow admins to choose a revenue model #4230

ChrisAntaki opened this issue Oct 15, 2021 · 12 comments
Labels
Module: Subscribe with Google Subscribe with Google module related issues P1 Medium priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Milestone

Comments

@ChrisAntaki
Copy link
Contributor

ChrisAntaki commented Oct 15, 2021

Feature Description

SwG shows different UX to website visitors, depending on what kind of monetization model the publisher chose. Currently there are two options: "subscription" and "contribution". The SwG module should let admins choose one


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

Acceptance criteria

  • The SwG module should support an additional revenueModel module setting which can be set to either subscription or contribution.
  • The setup flow and settings edit UI should include a dropdown to choose the revenueModel. The value of the setting should also be displayed in the settings view UI.
  • The user-facing label should be "Revenue model", and the UI elements related to this setting should display below "Publication ID", i.e. above "Products".
  • It should not be possible to complete the SwG setup or to save SwG settings without setting an revenueModel.
  • The revenueModel should be required to be set in order for the SwG module to be considered "connected".

Implementation Brief

  • Create a new Select component called RevenueModelInput
  • Update Swg's SettingsEdit component to show a loading indicator if the revenue model setting is undefined
  • Add RevenueModelInput component to SwG's SettingsForm component, and update corresponding Storybook
  • Display the revenue model on SwG's SettingsView component, and update corresponding Storybook
  • Add RevenueModelInput component to SwG's SetupForm component, and update corresponding Storybook
  • Register revenueModel in SwG's datastore/base class
  • Require revenue model to SwG's datastore/settings class (plus tests)
  • Validate the revenue model is either "contribution" or "subscription"
  • Only report SwG as "connected" if revenue model is set (PHP)
  • Register revenueModel setting, defaulting to an empty string (PHP)

Test Coverage

  • Update affected Storybook tests: SettingsForm, SettingsView, and SetupForm
  • Update the defaultSettings and validSettings in the SetupForm tests
  • Test the revenue model is required in the datastore/settings class
  • Update the main PHP module's tests to define the revenue model when eliciting a "connected" response

QA Brief

  • Enable the swgModule feature flag with the tester plugin
  • Click "Set up Subscribe with Google" on the Settings > Connect More Services page
  • Fill out the form
    • You can set the revenue model dropdown to either Contributions or Subscriptions

Changelog entry

  • Add revenue setting to Subscribe with Google module.
@felixarntz felixarntz added P1 Medium priority Module: Subscribe with Google Subscribe with Google module related issues Type: Enhancement Improvement of an existing feature labels Oct 15, 2021
@felixarntz felixarntz assigned ChrisAntaki and unassigned felixarntz Oct 15, 2021
@ChrisAntaki ChrisAntaki changed the title SwG module should allow admins to choose an autoPromptType SwG module should allow admins to choose a revenue model Oct 15, 2021
@felixarntz
Copy link
Member

@ChrisAntaki This IB is a bit hard to digest/review, because you started with a PR. The almost only thing to review here without looking at actual code is the first point, the proposed component name, which is always worth including in an IB. Other than that though, you're merely referencing file names where this somehow needs to be included. The IB should be more higher-level, something you can write before writing the PR. Looking at the code and file structure is typically needed for that, but you shouldn't get down to the level of mentioning a file here for example just because it needs that as an import or export etc. It's more about explaining the high-level approach.

Things that would be good to cover here for example would be:

  • You say "Add Revenue model to..." everywhere - for the PHP side it would be more precise to say something like "Register revenueModel setting in ... class", and include things like what to put as default value for example.
  • It would be worth describing how the validation in JS for that setting should behave.
  • The IB is also not mentioning any approach for covering the last bullet point of the ACs, ensuring that the module doesn't show as "connected" without revenueModel set.

The above would be points relevant to think through before writing the code.

@felixarntz felixarntz assigned ChrisAntaki and unassigned felixarntz Oct 18, 2021
@ChrisAntaki
Copy link
Contributor Author

Thanks for your feedback, I tried improving the IB based on it. Please let me know what you think

@felixarntz
Copy link
Member

Thanks @ChrisAntaki, looks good! IB ✅

@felixarntz felixarntz assigned ChrisAntaki and unassigned felixarntz Oct 18, 2021
@felixarntz felixarntz added this to the Sprint 60 milestone Oct 18, 2021
@felixarntz
Copy link
Member

felixarntz commented Oct 18, 2021

@ChrisAntaki Can you also add an estimate for the issue? When deciding on one, please factor in a high-level estimate of how much code there will be to review, including potential rounds of feedback - as in, just because something might take you 1 hour to initially come up with, don't just put a 1 on it, unless it's super-straightforward to implement, code-review and test (which essentially applies almost only to wording changes).

Other than that, this is now ready for engineering execution!

@ChrisAntaki
Copy link
Contributor Author

Sounds good! I estimated 7 to be safe

@felixarntz felixarntz removed their assignment Oct 21, 2021
@wpdarren wpdarren self-assigned this Oct 22, 2021
@wpdarren
Copy link
Collaborator

wpdarren commented Oct 22, 2021

QA Update: ❌

@ChrisAntaki I have a few observations.

  1. When you go to Settings > Connected Services, the option I selected is appearing but it's in lowercase. In the dropdown it's title case. Is that correct?

image

  1. The space between Publication ID and Revenue Model fields is bigger than between Products.

image

  1. I feel this is unrelated to this ticket, but wanted to highlight it. When you disconnect Subscribe with Google it appears to behave as expected, but when you go to reconnect the service, the data you entered previously re-appears. When you go back to settings, it is connected automatically.

This is a screencast. Would you like me to create a ticket for this?

swg.mp4

@wpdarren wpdarren removed their assignment Oct 22, 2021
@fhollis fhollis modified the milestones: Sprint 60, Sprint 61 Oct 25, 2021
@fhollis fhollis added the Rollover Issues which role over to the next sprint label Oct 25, 2021
@felixarntz
Copy link
Member

felixarntz commented Oct 25, 2021

@ChrisAntaki Can you address the three points by @wpdarren in a follow-up PR?

  1. This should be showing the same respective label that is used in the dropdown, based on the value currently set.
  2. Either some containers are misplaced here, or it may just be a CSS issue?
  3. This bug is happening because the SwG PHP module component isn't formally implementing the Module_With_Deactivation interface. It does have the respective on_deactivation() method, but is missing the interface implementation declaration. Once that's added, it should wipe the module data on deactivation as expected.

@ChrisAntaki
Copy link
Contributor Author

Nice finds, @wpdarren

I'll create the follow-up PR today, @felixarntz 👍

@felixarntz
Copy link
Member

@aaemnnosttv One question for you in #4281 (comment)

@felixarntz felixarntz removed their assignment Oct 27, 2021
@wpdarren wpdarren self-assigned this Oct 28, 2021
@wpdarren
Copy link
Collaborator

wpdarren commented Oct 28, 2021

QA Update: ❌

@ChrisAntaki @felixarntz this has passed the QAB and the previous issues fixed other than one observation.

The spacing between fields and button seems too big. If you look at my previous screenshots you will see the difference. If you're happy with the spacing that's fine, just wanted to check. Everything else is looking great.

image

Verified:

  • On the Subscribe with Google page, I can see a field Revenue model and when I click it, two options appear in the dropdown: contributions and subscriptions
  • Above this field is Publication ID and below is Products fields.

image

  • It is not be possible to complete the SwG setup or to save SwG settings without setting an revenueModel. The configure button is inactive.

image

  • Subscribe with Google is connected and all of the appropriate data appears in Settings > Connected Services

  • The value selected appears in the correct case in Settings > Connected Services.

image

  • Tested to make sure that the revenueModel setting could be changed and saved in Settings > Connected Services

@wpdarren wpdarren assigned ChrisAntaki and unassigned wpdarren Oct 28, 2021
@felixarntz
Copy link
Member

@wpdarren

The spacing between fields and button seems too big. If you look at my previous screenshots you will see the difference. If you're happy with the spacing that's fine, just wanted to check. Everything else is looking great.

Are you referring to the first screenshot in your comment? Just want to double-check, since in the other screenshot that particular spacing is less (and looks better), so I wonder whether those other screenshots were from a previous iteration.

In the first screenshot I agree though that the space above the button looks like too much. However, I'd say this is okay for now, especially since the eventual setup UI will look slightly different. Thanks for flagging it though!

Based on your feedback, I'll mark this as QA ✅ now.

@wpdarren
Copy link
Collaborator

@felixarntz yes, I was referring to the first screenshot. The screenshots below are what they looked like previously. Thanks for taking a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Subscribe with Google Subscribe with Google module related issues P1 Medium priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants