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

feat(ras): skip campaign setup #3051

Merged
merged 12 commits into from
Apr 25, 2024
Merged

feat(ras): skip campaign setup #3051

merged 12 commits into from
Apr 25, 2024

Conversation

jaredrethman
Copy link
Collaborator

@jaredrethman jaredrethman commented Apr 9, 2024

All Submissions:

Changes proposed in this Pull Request:

Ability to allow editors to skip "Reader Activation Campaign" setup.

How to test the changes in this Pull Request:

  1. Checkout this branch and build assets i.e. git checkout origin/feat/skip-ras-campaign-setup && npm run build
  2. Ensure you've completed the first 5 Reader Activation settings, example below:
    Screenshot 2024-04-09 at 15 28 39
  3. Expand "Reader Activation Campaign" and click "Update Reader Activation campaign" or navigate to - /wp-admin/admin.php?page=newspack-engagement-wizard#/reader-activation/campaign.
  4. Click (newly added) "Skip" button, at the bottom-right of the screen. Confirm prompt.
  5. Observe once response is received from server the "Continue" button is enabled.
  6. Navigate back to Newspack / Engagement or /wp-admin/admin.php?page=newspack-engagement-wizard#/reader-activation and confirm "Reader Activation Campaign" has status "Ready (Skipped)"
    Screenshot 2024-04-09 at 10 44 14

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@jaredrethman jaredrethman added the [Status] Needs Review The issue or pull request needs to be reviewed label Apr 10, 2024
@jaredrethman jaredrethman marked this pull request as ready for review April 10, 2024 03:33
@jaredrethman jaredrethman requested a review from a team as a code owner April 10, 2024 03:33
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaredrethman Thanks for diving into this without much context! The UI work is pretty much exactly what's needed aside from a minor suggestion below. However, we're missing two other requirements to truly skip the campaign setup:

  1. If skipping the Campaign step, we should not execute this call to Reader_Activation::activate(). This method is what actually generates the campaign prompts + segments, so to truly skip we shouldn't generate those at all. I suppose we could just set $response to true in this case.
  2. A more minor detail: if skipping, we should probably remove this step from the progress animation that shows when you click "activate" on the last step. If skipping campaign setup, we shouldn't be creating segments, so showing this step doesn't make sense.

The end result after skipping is that all prerequisites in the Reader Activation screen should be complete (as you've implemented already), but when visiting Newspack > Campaigns there shouldn't be anything here. Sorry if the business requirement wasn't clear—this feature is meant to support publishers who don't want to use the defaults at all.

return;
}
setSkipped( { isSkipped: Boolean( request.skipped ), status: '' } );
newspack_engagement_wizard.is_skipped_campaign_setup = request.skipped ? '1' : '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this could we automatically transition to the next view? I clicked the "Skip" button and didn't realize I still had to click "Continue" to proceed.

@jaredrethman
Copy link
Collaborator Author

@dkoo appreciate the review and feedback, but especially sharing insight into desired outcome. Some responses:

If skipping the Campaign step, we should not execute this call to Reader_Activation::activate(). This method is what actually generates the campaign prompts + segments, so to truly skip we shouldn't generate those at all. I suppose we could just set $response to true in this case.

public function api_activate_reader_activation( WP_REST_Request $request ) {
$skip_activation = $request->get_param( 'skip_activation' ) ?? false;
$response = $skip_activation ? true : Reader_Activation::activate();
if ( \is_wp_error( $response ) ) {
return new \WP_REST_Response( [ 'message' => $response->get_error_message() ], 400 );
}
if ( true === $response ) {
Reader_Activation::update_setting( 'enabled', true );
}
return rest_ensure_response( $response );
}

A more minor detail: if skipping, we should probably remove this step from the progress animation that shows when you click "activate" on the last step. If skipping campaign setup, we shouldn't be creating segments, so showing this step doesn't make sense.

if ( isSkippedCampaignSetup && activationSteps.length !== activationStepsCount - 1 ) {
activationSteps.shift();
}

but when visiting Newspack > Campaigns there shouldn't be anything here.

I've tested with latest and no Segments + Campaigns were created.

After this could we automatically transition to the next view? I clicked the "Skip" button and didn't realize I still had to click "Continue" to proceed.

Done 👍

Let me know if I missed anything and thanks again 😄

@jaredrethman jaredrethman requested a review from dkoo April 15, 2024 17:09
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaredrethman this is working really well now! Sorry about this, but I forgot about one other thing that doesn't make sense if you're skipping the campaign setup. In the /complete view there's a list of "what happens next" before you click the "Enable Reader Activation" button:

Screenshot 2024-04-15 at 1 10 16 PM

If you've skipped the campaign setup, could we add [skipped] to the steps that won't happen? Something like this:

Screenshot 2024-04-15 at 1 14 22 PM

@jaredrethman
Copy link
Collaborator Author

@jaredrethman this is working really well now! Sorry about this, but I forgot about one other thing that doesn't make sense if you're skipping the campaign setup. In the /complete view there's a list of "what happens next" before you click the "Enable Reader Activation" button:

Have updated with your feedback. Should now see the below, after your pull latest code with commit:feat: pr feedback, appended [skipped] to select items

Screenshot 2024-04-15 at 20 34 01

Let me know if there's anything else, thanks for such a thorough review!

@jaredrethman jaredrethman requested a review from dkoo April 16, 2024 01:37
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, @jaredrethman!

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Apr 16, 2024
@jaredrethman
Copy link
Collaborator Author

@dkoo just double checking the status here. Can this be merged (into trunk)?

@dkoo
Copy link
Contributor

dkoo commented Apr 23, 2024

Yes, it can be merged! Sorry I wasn't clear. Approval = good to merge

@dkoo
Copy link
Contributor

dkoo commented Apr 23, 2024

And when merging, please squash

@adekbadek
Copy link
Member

@jaredrethman – also when squashing, ensure the commit message follows the <type>(<domain>): <message> pattern – in this case: feat(reader-activation): allow skipping campaigns setup.

@dkoo dkoo merged commit 9ef0e6d into trunk Apr 25, 2024
7 checks passed
@dkoo dkoo deleted the feat/skip-ras-campaign-setup branch April 25, 2024 15:51
@dkoo
Copy link
Contributor

dkoo commented Apr 25, 2024

Went ahead and merged so it makes it into the next release cycle!

matticbot pushed a commit that referenced this pull request Apr 25, 2024
# [4.0.0-alpha.1](v3.8.0...v4.0.0-alpha.1) (2024-04-25)

### Bug Fixes

* ensure only admins can reset starter content and newspack options ([#3081](#3081)) ([4606721](4606721))

### Features

* add product option to autocomplete orders ([#3072](#3072)) ([4a2859b](4a2859b))
* **ras:** skip campaign setup ([#3051](#3051)) ([9ef0e6d](9ef0e6d))

### BREAKING CHANGES

* Changes order autocompletion behavior for existing products!

* feat: add product option to autocomplete orders

* chore: update outdated docblock description
@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.0.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

claudiulodro pushed a commit that referenced this pull request May 15, 2024
* chore(deps-dev): bump @wordpress/browserslist-config

Bumps [@wordpress/browserslist-config](https://github.com/WordPress/gutenberg/tree/HEAD/packages/browserslist-config) from 5.37.0 to 5.38.0.
- [Release notes](https://github.com/WordPress/gutenberg/releases)
- [Changelog](https://github.com/WordPress/gutenberg/blob/trunk/packages/browserslist-config/CHANGELOG.md)
- [Commits](https://github.com/WordPress/gutenberg/commits/@wordpress/browserslist-config@5.38.0/packages/browserslist-config)

---
updated-dependencies:
- dependency-name: "@wordpress/browserslist-config"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* chore(deps-dev): bump @rushstack/eslint-patch from 1.10.1 to 1.10.2

Bumps [@rushstack/eslint-patch](https://github.com/microsoft/rushstack/tree/HEAD/eslint/eslint-patch) from 1.10.1 to 1.10.2.
- [Changelog](https://github.com/microsoft/rushstack/blob/main/eslint/eslint-patch/CHANGELOG.md)
- [Commits](https://github.com/microsoft/rushstack/commits/@rushstack/eslint-patch_v1.10.2/eslint/eslint-patch)

---
updated-dependencies:
- dependency-name: "@rushstack/eslint-patch"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* chore(deps-dev): bump @types/wordpress__blocks from 12.5.13 to 12.5.14

Bumps [@types/wordpress__blocks](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/wordpress__blocks) from 12.5.13 to 12.5.14.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/wordpress__blocks)

---
updated-dependencies:
- dependency-name: "@types/wordpress__blocks"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* refactor(data-events): add abstract Connector class for shared methods (#3015)

* refactor: add abstract Connector class for shared methods

* chore: remove unused imports

* chore(deps-dev): bump @wordpress/browserslist-config

Bumps [@wordpress/browserslist-config](https://github.com/WordPress/gutenberg/tree/HEAD/packages/browserslist-config) from 5.38.0 to 5.39.0.
- [Release notes](https://github.com/WordPress/gutenberg/releases)
- [Changelog](https://github.com/WordPress/gutenberg/blob/trunk/packages/browserslist-config/CHANGELOG.md)
- [Commits](https://github.com/WordPress/gutenberg/commits/@wordpress/browserslist-config@5.39.0/packages/browserslist-config)

---
updated-dependencies:
- dependency-name: "@wordpress/browserslist-config"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* chore(deps): bump qs and @types/qs

Bumps [qs](https://github.com/ljharb/qs) and [@types/qs](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/qs). These dependencies needed to be updated together.

Updates `qs` from 6.12.0 to 6.12.1
- [Changelog](https://github.com/ljharb/qs/blob/main/CHANGELOG.md)
- [Commits](ljharb/qs@v6.12.0...v6.12.1)

Updates `@types/qs` from 6.9.14 to 6.9.15
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/qs)

---
updated-dependencies:
- dependency-name: qs
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: "@types/qs"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* fix: ensure only admins can reset starter content and newspack options (#3081)

* feat: add product option to autocomplete orders (#3072)

BREAKING CHANGE: Changes order autocompletion behavior for existing products!

* feat: add product option to autocomplete orders

* chore: update outdated docblock description

* feat(ras): skip campaign setup (#3051)

* feat(ras): skip campaign setup boilerplate

added new route, added fe logic

* feat(ras): added `is_skipped` to prerequisite status endpoint

* refactor: moved callback to named function

added ui to indicate async skipping flow

* feat: added skip logic to prerequisite component

combined "skipped" and "active" into single `isValid` const

* feat: `is_skipped` property to prerequisite type

added skip logic to RAS wizard

* fix: circle ci / typescript

* fix: ci / eslint

* refactor: remove redirect after skip

added allow continue when skipped.

* refactor: allow parametrized skip

* refactor: revert any type cast

* feat: pr feedback - added redirect, activation skip and remove `activationStep[0]`

* feat: pr feedback, appended `[skipped]` to select items

* chore(release): 4.0.0-alpha.1 [skip ci]

# [4.0.0-alpha.1](v3.8.0...v4.0.0-alpha.1) (2024-04-25)

### Bug Fixes

* ensure only admins can reset starter content and newspack options ([#3081](#3081)) ([4606721](4606721))

### Features

* add product option to autocomplete orders ([#3072](#3072)) ([4a2859b](4a2859b))
* **ras:** skip campaign setup ([#3051](#3051)) ([9ef0e6d](9ef0e6d))

### BREAKING CHANGES

* Changes order autocompletion behavior for existing products!

* feat: add product option to autocomplete orders

* chore: update outdated docblock description

* chore(release): 4.0.0-alpha.2 [skip ci]

# [4.0.0-alpha.2](v4.0.0-alpha.1...v4.0.0-alpha.2) (2024-04-25)

### Bug Fixes

* remove deprecated filter callback ([#3090](#3090)) ([5d7d0bf](5d7d0bf))

* chore(release): 4.0.0-alpha.3 [skip ci]

# [4.0.0-alpha.3](v4.0.0-alpha.2...v4.0.0-alpha.3) (2024-04-26)

### Bug Fixes

* enable Memberships fix cron job only when environment constant is defined ([#3087](#3087)) ([5d40297](5d40297))

* OAuth: use a custom table for transients (#3106)

* feat: start new custom table for oauth transients data

* fix: copy/paste error

* fix(google-oauth): use custom table for transients

* feat: cleanup old transients

* fix: clean up on the fly too, and limit to deleting 1000 at a time max

---------

Co-authored-by: Adam Cassis <adam.cassis@automattic.com>

* fix: autocomplete orders only for virtual products (#3111)

* fix: autocomplete orders only for virtual products

* chore: remove console.log

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: matticbot <sysops+ghmatticbot@automattic.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Derrick Koo <dkoo@users.noreply.github.com>
Co-authored-by: Jared Rethman <jaredrethman@gmail.com>
Co-authored-by: matticbot <semantic-release-bot@martynus.net>
Co-authored-by: dkoo <derrick.koo@automattic.com>
@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.0.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request May 28, 2024
# [4.0.0](v3.8.8...v4.0.0) (2024-05-28)

### Bug Fixes

* autocomplete orders only for virtual products ([#3111](#3111)) ([bfbe554](bfbe554))
* **data-events:** handle no ga_client_id in data ([fc4fc9f](fc4fc9f))
* ensure only admins can reset starter content and newspack options ([#3081](#3081)) ([4606721](4606721))
* **registration-block:** prevent undefined variable warning ([8af9d89](8af9d89))
* **starter-content:** make the starter content generation idempotent ([d5e10ff](d5e10ff))

### Features

* add product option to autocomplete orders ([#3072](#3072)) ([4a2859b](4a2859b))
* **cli:** command to migrate CAP guest authors to WP users ([#3068](#3068)) ([7ea8273](7ea8273))
* custom role for assignable authors who don't edit posts ([#3066](#3066)) ([7b89053](7b89053))
* **ras:** skip campaign setup ([#3051](#3051)) ([9ef0e6d](9ef0e6d))
* support for wrapping rss titles in cdata ([#3104](#3104)) ([8b3a3be](8b3a3be))

### BREAKING CHANGES

* Changes order autocompletion behavior for existing products!

* feat: add product option to autocomplete orders

* chore: update outdated docblock description
@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @alpha released on @epic/ras-acc released [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants