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.

Sign up for GitHub

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

Jump to bottom

Adds b2bEnabled enabledFeature on CustomerSegmentTemplateApi #1361

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Adds b2bEnabled enabledFeature on CustomerSegmentTemplateApi #1361

merged 1 commit into from
Sep 20, 2023

Conversation

lauramann
Copy link
Contributor

@lauramann lauramann commented Sep 19, 2023 •
edited

Background

Resolves https://github.com/Shopify/core-issues/issues/60600 (more context in https://github.com/Shopify/core-issues/issues/58095)

We want to have a template in customer segmentation that only appears in shops on the plus plan.

Solution

  • Adds b2bEnabled enabledFeature to our API
  • Removes other enabledFeatures we are no longer using. The beta flags for these features have been removed.
  • Cleans up InternalCustomerSegmentTemplate example to use b2bEnabled enabledFeature instead of productPurchasedOnTags
  • Cleans up icon list as buyButtonMajor was listed twice

🎩

  • Nothing to tophat. We aren't even using enabledFeatures in our 1p app or in web currently.
  • Green CI 🟢

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

lauramann
lauramann commented Sep 19, 2023
View reviewed changes
.../ui-extensions/src/surfaces/admin/api/customer-segment-template/customer-segment-template.ts
@@ -4,10 +4,8 @@ import type {ExtensionTarget as AnyExtensionTarget} from '../../extension-target

/* List of enabled query language features during a progressive rollout */
type CustomerSegmentationFeature =
/* Allows merchants to segment on products purchased by tags. For example: products_purchased(tag: 'Red hats') = true */
| 'productsPurchasedByTags'
Copy link
Contributor Author

@lauramann lauramann Sep 19, 2023 •
edited

Choose a reason for hiding this comment

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

products_purchased_on_tags_enabled no longer exists

AFAIK the aggregateFilters feature was never used, and this is fully rolled out now (@LTiger14 can you confirm?)

I'm assuming we won't need these features in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't even know we had that. We can remove it

lauramann
lauramann commented Sep 19, 2023
View reviewed changes
...surfaces/admin/components/InternalCustomerSegmentTemplate/InternalCustomerSegmentTemplate.ts
@@ -21,8 +21,7 @@ export type CustomerSegmentTemplateIcon =
| 'buyButtonMajor'
| 'followUpEmailMajor'
| 'confettiMajor'
| 'viewMajor'
| 'buyButtonMajor';
Copy link
Contributor Author

@lauramann lauramann Sep 19, 2023 •
edited

Choose a reason for hiding this comment

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

buyButtonMajor is listed twice

@lauramann lauramann changed the title updates enabledFeatures on CustomerSegmentTemplateApi Adds shopifyPlusPlan enabledFeature on CustomerSegmentTemplateApi Sep 19, 2023
@lauramann lauramann marked this pull request as ready for review September 19, 2023 18:34
@github-actions

This comment has been minimized.

Sign in to view
@lauramann lauramann force-pushed the lm/update-customer-template-api branch 2 times, most recently from 2cfb589 to 7fc0e51 Compare September 19, 2023 18:36
ashcharles
ashcharles approved these changes Sep 20, 2023
View reviewed changes
Copy link

@ashcharles ashcharles left a comment

Choose a reason for hiding this comment

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

Minor detail about the exact flag but very good with the example :)

.changeset/curvy-turkeys-leave.md
@@ -0,0 +1,5 @@
---
Copy link

Choose a reason for hiding this comment

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

I'm amused by curvy-turkeys-leave

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm always a little excited to see what the changeset file is called 😆 🦃

...mponents/InternalCustomerSegmentTemplate/examples/InternalCustomerSegmentTemplate.example.ts Outdated
category: 'reEngageCustomers',
});
const shopifyPlusPlanEnabled =
__enabledFeatures.includes('shopifyPlusPlan');
Copy link

Choose a reason for hiding this comment

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

fwiw, the actual restriction on this template should be is b2b enabled on the shop which is mostly equivalent to being on Plus except on some dev/partner shops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the callout @ashcharles! In the figma mockups of the template, the rule of visibility was based on whether the shop was on a plus plan or not. I can make this more specific to b2b!

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have much context around b2b but based on what Ash mentioned, I agree this flag should be more b2b specific than just plus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the name to b2bEnabled 👍

LTiger14
LTiger14 approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@LTiger14 LTiger14 left a comment

Choose a reason for hiding this comment

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

LGTM! Just a small note about the name. Thanks for the cleanup as well!

@lauramann lauramann force-pushed the lm/update-customer-template-api branch from 7fc0e51 to c2a1b82 Compare September 20, 2023 16:09
@lauramann lauramann force-pushed the lm/update-customer-template-api branch from c2a1b82 to 0ca94ea Compare September 20, 2023 16:09
@lauramann lauramann changed the title Adds shopifyPlusPlan enabledFeature on CustomerSegmentTemplateApi Adds b2bEnabled enabledFeature on CustomerSegmentTemplateApi Sep 20, 2023
@lauramann lauramann merged commit 82ba4be into unstable Sep 20, 2023
5 checks passed
@lauramann lauramann deleted the lm/update-customer-template-api branch September 20, 2023 16:11
@shopify-github-actions-access shopify-github-actions-access bot mentioned this pull request Oct 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashcharles ashcharles ashcharles approved these changes

@LTiger14 LTiger14 LTiger14 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lauramann @ashcharles @LTiger14