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

fix: use loose type comparison for list__in and list__not_in #1326

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Jul 11, 2024

All Submissions:

Changes proposed in this Pull Request:

Currently list__in and list__not_in criteria matching functions use includes (which enforces strict type comparison) to compare segmentation criteria values with reader data values. However, we've seen varying behavior across different sites where IDs get stored in reader data as strings instead of integers, which causes these matching functions to fail.

This change allows the type comparison to be loose using ==, which should solve the issue.

How to test the changes in this Pull Request:

Unit tests should prove this, but to test functionally:

  1. On release, create a segment and choose a product in the "Does not have active subscription" option. Publish a prompt to that segment.
  2. Register a reader account and purchase the required subscription product. Visit at least a couple of pages to ensure that the reader data updates in localStorage and confirm that you don't see the prompt you created in step 1.
  3. In dev tools, go to Application > Storage, unfurl "Local storage", and click on your dev site's URL. Note the np_reader_1_active_subscriptions value. If it's an array containing an integer click the value to edit it to a string with the same number, like so:
Screenshot 2024-07-11 at 2 55 44 PM
  1. Visit a couple more pages and confirm that you start seeing the prompt you created in step 1 (because your reader data value is "275" and the segment expects 275).
  2. Check out this branch, visit another page and confirm you no longer see the prompt.

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?

@dkoo dkoo self-assigned this Jul 11, 2024
@dkoo dkoo changed the base branch from trunk to release July 11, 2024 20:57
@dkoo dkoo marked this pull request as ready for review July 11, 2024 20:58
@dkoo dkoo requested a review from a team as a code owner July 11, 2024 20:58
@dkoo dkoo merged commit 1872c96 into release Jul 12, 2024
8 checks passed
@dkoo dkoo deleted the fix/loose-comparison-matching branch July 12, 2024 17:10
matticbot pushed a commit that referenced this pull request Jul 12, 2024
## [2.34.1](v2.34.0...v2.34.1) (2024-07-12)

### Bug Fixes

* use loose type comparison for list__in and list__not_in ([#1326](#1326)) ([1872c96](1872c96))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.34.1 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants