[HOLD] feat: default segments #809

dkoo · 2022-02-23T00:12:07Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Starts the "reasonable defaults" part of the roadmap. Allows site admins to generate a set of default segments with a button click. If they've already been created and edited, clicking the button will restore the default segments to the default configuration.

The reason for requiring some kind of user action (in this case a button click) is to accommodate sites that may have been up and running with Campaigns for a while now—we don't want to automatically generate new segments which may conflict with existing segments that are running live prompts.

Closes #673.

How to test the changes in this Pull Request:

Before testing, make sure your Newspack Plugin is running Automattic/newspack-plugin#1524.

On a fresh or existing site, visit Newspack > Campaigns > Segments. Confirm there's a new button at the bottom of the segments list:

Click the button. Confirm you see a modal with a bit of explanation of what you're doing (@aschweigert: could use your help with reviewing/updating this copy):

Click "OK". Confirm that a set of default segments (with prefix "[Newspack]") is generated. If you already had existing segments on the site, confirm that the default segments are generated with lower priority than the existing segments.
Edit some of the default segment names and settings, and reshuffle the priority. Delete at least one default segment.
Click "Reset default segments" and click "OK". Confirm that:

All existing segments retain their sorted priority
The default segment(s) you edited are now reverted to their original names and settings
The default segment(s) you deleted are restored

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?

…h a click Starts the "reasonable defaults" part of the roadmap. Allows site admins to generate a set of default segments with a button click. If they've already been created and edited, clicking the button will restore the default segments to the default configuration. #674

adekbadek

The reason for requiring some kind of user action (in this case a button click) is to accommodate sites that may have been up and running with Campaigns for a while now—we don't want to automatically generate new segments which may conflict with existing segments that are running live prompts.

What if there are no segments on the site – should defaults be set up then? This would be a surprise in the "Campaigns" tab, but there would be no disruption to the existing campaigns setup. WDYT @dkoo @aschweigert ?

includes/class-newspack-popups-segmentation.php

dkoo · 2022-02-23T19:09:57Z

What if there are no segments on the site – should defaults be set up then? This would be a surprise in the "Campaigns" tab, but there would be no disruption to the existing campaigns setup. WDYT @dkoo @aschweigert ?

I can get behind this. Maybe if there are no segments AND no active prompts? This would avoid creating confusion for sites that may have active prompts running for "everyone" and don't intend to use the segmentation features.

EDIT: further, we could trigger the auto-generation when you navigate to the Campaigns wizard, this way we could show a short message explaining what happened. And if a site admin never visits Campaigns, we don't need to run the extra functions.

… segment

dkoo · 2022-02-23T21:56:27Z

@adekbadek I pushed some changes to address your feedback. I think the auto-generation we're discussing can probably live in the main plugin if we trigger it upon rendering the Campaigns wizard.

adekbadek · 2022-02-24T10:31:50Z

includes/class-newspack-popups-segmentation.php

+				],
+			],
+			'everyone_else'    => [
+				'name'          => __( ' Other Readers', 'newspack-popups' ),


Nit: there are extra leading spaces in the segments names:

Suggested change

'name' => __( ' Other Readers', 'newspack-popups' ),

'name' => __( 'Other Readers', 'newspack-popups' ),

Oops, fixed in 86337ae.

includes/class-newspack-popups-segmentation.php

adekbadek · 2022-02-24T10:55:57Z

Maybe if there are no segments AND no active prompts? This would avoid creating confusion for sites that may have active prompts running for "everyone" and don't intend to use the segmentation features.

Adding the default segments won't disrupt their prompt delivery in any way, and I think the user should be encouraged to make use of segmentation. So I'd say the defaults should be created if there are no segments, regardless of whether there are prompts.

we could trigger the auto-generation when you navigate to the Campaigns wizard, this way we could show a short message explaining what happened. And if a site admin never visits Campaigns, we don't need to run the extra functions.

Seems to me it'd be simpler to create the default segments on WP initialisation, without the extra API call. But it's not a strong opinion.

dkoo · 2022-02-24T18:56:46Z

a315d73 implements the auto-generation if the site doesn't have any segments.

includes/class-newspack-popups-segmentation.php

…then deleted

dkoo · 2022-02-25T17:06:32Z

Note to @thomasguillot: after some discussion we've decided we'd like to build this out into a more guided onboarding/wizard experience to guide users toward one of several sets of default segments, based on their input. @aschweigert will work on a suggested flow for this, but let's hold these changes until we can flesh that out a bit more.

adekbadek

Approving in the current state – as noted above it's likely to change, though.

dkoo · 2022-04-11T18:08:09Z

After some internal discussion we're going to be moving in a different direction for this. I'm going to close this for now—we can reopen if we change our minds about wanting to release it as an interim step.

dkoo self-assigned this Feb 23, 2022

dkoo added enhancement New feature or request Feature request [Status] Needs Review and removed enhancement New feature or request labels Feb 23, 2022

dkoo added this to the 2 - Better defaults milestone Feb 23, 2022

dkoo requested a review from aschweigert February 23, 2022 00:19

dkoo mentioned this pull request Feb 23, 2022

[HOLD] feat(popups): default segments UI Automattic/newspack-plugin#1524

Closed

6 tasks

dkoo changed the title ~~feat(segments): add ability to generate or reset default segments wit…~~ feat: default segments Feb 23, 2022

dkoo marked this pull request as ready for review February 23, 2022 00:24

dkoo requested a review from a team as a code owner February 23, 2022 00:24

adekbadek requested changes Feb 23, 2022

View reviewed changes

dkoo added 2 commits February 23, 2022 14:45

refactor: no prefix for default segment names; rename "everyone else"…

2c4d923

… segment

refactor: update name of $return_id variable , for clarity

debe5f6

dkoo requested a review from adekbadek February 23, 2022 21:55

adekbadek reviewed Feb 24, 2022

View reviewed changes

dkoo added 2 commits February 24, 2022 11:26

refactor: use canonize_segment method

86337ae

feat: automatically create default segments if no segments exist

a315d73

dkoo requested a review from adekbadek February 24, 2022 18:56

adekbadek requested changes Feb 25, 2022

View reviewed changes

includes/class-newspack-popups-segmentation.php Outdated Show resolved Hide resolved

includes/class-newspack-popups-segmentation.php Outdated Show resolved Hide resolved

dkoo added 2 commits February 25, 2022 09:46

chore: remove unwanted leading spaces

794ac04

fix: do not create default segments if they were already created and …

0aaae26

…then deleted

dkoo requested a review from adekbadek February 25, 2022 16:49

dkoo added the [Status] On Hold label Feb 25, 2022

dkoo changed the title ~~feat: default segments~~ [HOLD] feat: default segments Feb 25, 2022

adekbadek approved these changes Feb 28, 2022

View reviewed changes

github-actions bot added [Status] Approved and removed [Status] Needs Review labels Feb 28, 2022

dkoo closed this Apr 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD] feat: default segments #809

[HOLD] feat: default segments #809

dkoo commented Feb 23, 2022 •

edited

Loading

adekbadek left a comment

dkoo commented Feb 23, 2022 •

edited

Loading

dkoo commented Feb 23, 2022

adekbadek Feb 24, 2022

dkoo Feb 24, 2022

adekbadek commented Feb 24, 2022

dkoo commented Feb 24, 2022

dkoo commented Feb 25, 2022

adekbadek left a comment

dkoo commented Apr 11, 2022

	'name' => __( ' Other Readers', 'newspack-popups' ),
	'name' => __( 'Other Readers', 'newspack-popups' ),

[HOLD] feat: default segments #809

[HOLD] feat: default segments #809

Conversation

dkoo commented Feb 23, 2022 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

adekbadek left a comment

Choose a reason for hiding this comment

dkoo commented Feb 23, 2022 • edited Loading

dkoo commented Feb 23, 2022

adekbadek Feb 24, 2022

Choose a reason for hiding this comment

dkoo Feb 24, 2022

Choose a reason for hiding this comment

adekbadek commented Feb 24, 2022

dkoo commented Feb 24, 2022

dkoo commented Feb 25, 2022

adekbadek left a comment

Choose a reason for hiding this comment

dkoo commented Apr 11, 2022

dkoo commented Feb 23, 2022 •

edited

Loading

dkoo commented Feb 23, 2022 •

edited

Loading