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: favorite categories segmentation logic #1177

Merged
merged 6 commits into from
Jul 28, 2023

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Jul 27, 2023

All Submissions:

Changes proposed in this Pull Request:

Proposes logic for segmenting by favorite category. Picking up a thread from #1155 (comment). Note that I was slightly incorrect in my description of the logic in that thread; I missed the key() function in the current logic which means that the criteria categories are only compared against the top-most-viewed category.

The way the current (non-rearchitected) logic works is that a category is considered "favorite" if it's been viewed more times than any other category. This could result in a slightly awkward situation where if the very first post visited matches the segment criteria, it would make for a match on that very first pageview.

The proposed logic is almost the same, but tweaks it so that a category isn't considered favorite unless the reader has viewed at least two different articles, either in the same category or different categories. If the reader has viewed multiple articles in different categories, the top category must actually have more views than the other categories in order to be considered a favorite. This prevents the criteria from matching if, for example, the reader only views a single article in several different categories (in which case none can be considered a favorite).

How to test the changes in this Pull Request:

  1. Create a segment at top priority and assign several categories to the "Favorite categories" criteria.
  2. On the front-end, view some articles that don't match any of the categories in the segment. Confirm that you don't match the segment.
  3. In the same session, view multiple articles in one of the segment's categories (more than you viewed in step 2). Confirm that you do match the segment now that you've viewed more articles in at least one of the segment's categories than any other category you've viewed.
  4. In a new session, view at least two articles in any of the segment's categories. Confirm that you match the segment after viewing at least two articles in one of the segment's categories. This is so that readers who only ever view articles from a single category can still match the segment.

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 27, 2023
@dkoo dkoo requested a review from a team as a code owner July 27, 2023 23:28
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

It's working as described!

Non-blocking NIT, I think the same logic could be applied with a simpler statement:

views.length > 1 && countsArray[ 0 ][ 1 ] > countsArray[ 1 ][ 1 ]

I also came across an issue that I'm not sure we should handle. I have a "Featured" category in my local instance that is always on top during my tests because it's prioritized on the homepage. It's always assigned with other categories that are actual topics, but my "favorite category" doesn't work as "favorite topic" when using categories in this way.

Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

Whoops! Ran into an issue on a fresh session. It's trying to access the index of a countsArray item when countsArray is empty:

Uncaught TypeError: Cannot read properties of undefined (reading '1')
    at Object.eval [as matchingFunction] (favorite-categories.js:27:92)

@dkoo
Copy link
Contributor Author

dkoo commented Jul 28, 2023

845a854 should simplify the logic as suggested, as well as fix the case where the countsArray var is undefined.

I have a "Featured" category in my local instance that is always on top during my tests because it's prioritized on the homepage. It's always assigned with other categories that are actual topics, but my "favorite category" doesn't work as "favorite topic" when using categories in this way.

Can you describe how to set up categories in this way? And what does your segment's configuration look like? How can I replicate the issue?

@miguelpeixe
Copy link
Member

Can you describe how to set up categories in this way? And what does your segment's configuration look like? How can I replicate the issue?

It's how our bootstrap content is set up. We have a category named "Featured" that has demo posts attached to it:

image

While navigating to do my local tests, the "Featured" category was ranked at the top. It's not a "topic", so it defeats the purpose of the "favorite categories" segmentation. My favorite category can't be "featured".

I don't think there's a way around this. Anytime a publisher uses a category for other reason that is not a topic in combination with categories that are topics, there's a risk to not have the feature working as expected. Non-topic categories at the top.

@miguelpeixe
Copy link
Member

miguelpeixe commented Jul 28, 2023

Just realized that my suggestion can also fail if the reader has only visited a single category in multiple views. It'll try to get an index of undefined countsArray[1]

This should work:

1 < views.length && ( ! countsArray[ 1 ] || countsArray[ 0 ][ 1 ] > countsArray[ 1 ][ 1 ] )

Or, to save a little processing power, the views.length can move up to line 4 and return early so no unnecessary category calculation logic is executed and this check can be just ! countsArray[ 1 ] || countsArray[ 0 ][ 1 ] > countsArray[ 1 ][ 1 ]

@dkoo
Copy link
Contributor Author

dkoo commented Jul 28, 2023

Ah, so it's more about how different publishers might use categories for things other than topics, then? I think that's something we can't really control for. IMO as long as the feature works consistently for any category term, it's up to the publisher to create segments that make sense for their readership.

@dkoo
Copy link
Contributor Author

dkoo commented Jul 28, 2023

Ah, just saw your last comment. Will test your latest suggestion!

@dkoo
Copy link
Contributor Author

dkoo commented Jul 28, 2023

How's 855d094?

EDIT: plus f5d14ff, whoops

@dkoo dkoo requested a review from miguelpeixe July 28, 2023 20:25
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

Working as described! 🙌

@dkoo
Copy link
Contributor Author

dkoo commented Jul 28, 2023

Thanks, Miguel!

@dkoo dkoo merged commit 418cef9 into epic/rearchitecture Jul 28, 2023
1 check passed
@dkoo dkoo deleted the feat/favorite-categories-logic branch July 28, 2023 20:59
dkoo added a commit that referenced this pull request Aug 17, 2023
* feat: segmentation criteria system (#1155)

* chore: remove Lightweight API and client ID handling (#1166)

* chore: remove Lightweight API and client ID handling

* chore: remove data pruning cron job and clear future instances

* fix: script dependencies

* fix: remove unused method refs

* test: remove tests using deprecated features

* chore: remove more unneeded files

* test: remove other failing tests

* fix(criteria): warn when criteria value is not available (#1167)

* feat: integrate criteria with segments (#1159)

Co-authored-by: Leo Germani <[email protected]>

* fix(criteria): allow async matching configuration (#1175)

* feat: segmentation match logic (#1169)

* feat: segmentation criteria system

* chore: refactor default criteria and implement utils

* chore: api tweaks

* fix: range logic

* chore: isolate segment matching example

* fix: ras dependency placement

* fix: add referrer sources category

* fix: api tweaks for consistency

* chore: file structure

* chore: inline docs

* chore: api tweaks

* fix: schema tweaks

* feat: add schema and migration helper

* fix: rename newsletter subscriber store item

* fix: send segment config instead of value

* chore: move default criteria registration

* fix: class name

* fix: change criteria options values

* fix: tweak migration

* fix: remove referrer creation

* fix: remove referrer creation

* test: dont enqueue scripts on tests

* test: add criteria

* test: add required

* test: add criteria

* fix: logic typo

* test: frontend matching

* fix: range config

* fix: missing range can be any range

* test: php criteria registration

* test: fix php test

* test: fix typo

* chore: improve inline docs

* fix: update segment criteria schema

* fix: configuration migration

* feat: use criteria to match prompt segments

* feat: fix schema and tests

* refactor: file structure

* chore: remove AMP polyfill JS

* feat: implement scroll triggers and frequency

* fix: add .phpunit.result.cache to .gitignore

* refactor: use prompt and segment config via JSON instead of DOM

* test: remove unit tests related to AMP Access and GA3 analytics

* fix: only show one overlay per page

* revert: use prompt and segment config via JSON instead of DOM

* test: re-fix failing tests

* fix: close overlay on background tap

* fix: src should not be added to release packages

* test: unit tests for segmetnation API

* test: actual unit tests for segmentation API

* chore: fix inline comment for accuracy

* fix: race condition when using perfmatters

* refactor: trigger seen GA4 event based on prompt_seen activity

* refactor: make the 1x overlay at a time rule testable

* refactor: overlay display check according to new unit test

* test: update test assertion for accuracy

* fix(criteria): allow async matching configuration

* fix: ensure global

* fix: logic for one-at-a-time overlays

* Revert "fix: race condition when using perfmatters"

This reverts commit c9fab16.

* fix: remove defaultCriteria.js

* fix: race condition with reader library JS

---------

Co-authored-by: Miguel Peixe <[email protected]>
Co-authored-by: Leo Germani <[email protected]>

* feat: favorite categories segmentation logic (#1177)

* feat: logic for favorite categories

* fix: top-ranked category must actually have more views than the other categories

* fix: simplify logic and account for undefined countsArray

* fix: account for multiple views in one category; simplify logic a bit

* fix: correct indexOf comparison

* feat: migrate reader data (#1176)

* fix: a PHP warning when migrating a user to the new data schema (#1180)

* Refactor/segments relationship (#1168)

* feat: refactor segment relationship

* feat: refactor complete and tests updated

* fix: popup exporter

* fix: popup analytics events

* fix: avoid warning in the schema

This is actually a bug in core.
see https://core.trac.wordpress.org/ticket/56494

* fix: do not return disabled segments on REST

---------

Co-authored-by: Miguel Peixe <[email protected]>

* feat: Update/remove category cleanup (#1185)

* fix: remove old category cleanup

It was looking at deprecated configuration meta
After the refactor having a non existent cat should not be a problem

* test: remove test

* chore: remove lightweight api setup (#1183)

* fix: fallback if no newspack plugin (#1179)

* fix: allow prompts to be displayed without segmentation if no Newspack Plugin

* refactor: avoid use of negative variable name

* fix: newspack_popups_view.segments is not an array

* fix: hide segments taxonomy

---------

Co-authored-by: Leo Germani <[email protected]>
Co-authored-by: dkoo <[email protected]>

* fix: restore support for plugin settings (#1181)

* fix: support segmentation on donor landing page

* fix: support Mailchimp donor merge fields for segmentation

---------

Co-authored-by: Leo Germani <[email protected]>

* fix: lines added by accident in last merge

* feat: move migration methods (#1184)

Co-authored-by: Derrick Koo <[email protected]>

* fix: auto-fill segment (#1188)

* feat: remove non interactive mode (#1190)

* fix: missing release files (#1191)

* fix: distignore should include some src subfolders

* fix: segments admin URL localized key name

* fix: previews (#1178)

* fix: allow manual prompts to be displayed in previews

* fix: previewing as a segment

* feat: use temporary reader data sessions for previews

* fix: single-prompt previews

* refactor: use helper function for determining override

* test: add tests for view_as and pid overrides

* test: fix query string in test

---------

Co-authored-by: Miguel Peixe <[email protected]>
Co-authored-by: Leo Germani <[email protected]>
Co-authored-by: Miguel Peixe <[email protected]>
Co-authored-by: dkoo <[email protected]>
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.

2 participants