chore: remove Lightweight API and client ID handling #1166

dkoo · 2023-07-13T23:40:23Z

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:

Removes the lightweight PHP API and associated unit tests. Also removes other major features that will no longer be needed after the rearchitecture:

AMP client ID handling (and all functionality related to setting and getting the AMP client ID via cookies): This is no longer needed in this plugin, since all reader data and segmentation features will be tied to either browser session via localStorage, or to a WP user
AMP Analytics markup and GA3/UA analytics tracking via AMP Analytics: GA3 has been deprecated as of July 1, so there's no need to retain these methods, which send events only via GA3
AMP Access config and handling: This will be replaced by a pure JS solution in a future PR
Setting of reader donation/subscription/registration events: This is now handled by the Reader Data library in the main plugin
The data pruning cron job that cleans up temp reader events from the legacy tables: No longer needed since we won't be writing to these tables anymore.

Note that this PR does extensive cleanup that will intentionally result in broken functionality. The following features are broken and will need to be reimplemented in separate PRs using the criteria system introduced in #1155:

Segmentation: In previews and on the front-end, all prompts will be visible at all times. The prompts will need to be hooked up with the new criteria system.
Previewing: The new criteria system does not implement per-prompt or per-segment previewing. This will need to be reintroduced using the new system. I left the view-as parameter setting and parsing intact in case we want to reuse those in the new system.
Segment reach estimates: This feature in the Campaigns dashboard UI relies on reader data existing in the SQL database, and will no longer work. A separate PR in newspack-plugin will need to deprecate this feature in that plugin.

This PR also doesn't do some further cleanup which will be done after we have a means to migrate legacy reader data to the new system. Notably, we're not cleaning up any reader data in the custom tables (although we're no longer writing any new data to those tables). There may also be methods related to AMP cookies and client IDs across our other repos that we can delete, too, but I'm choosing to leave the client ID handling in other repos intact until we're sure they're no longer needed in those plugins.

Note also that some changes are required in other Newspack plugins to remove references to methods that will no longer exist after this changeset. Automattic/newspack-plugin#2558 and Automattic/newspack-blocks#1498 are works-in-progress which we can add more changes to if necessary for the rearchitecture.

Closes 1204751246381647/1205010980866823.

How to test the changes in this Pull Request:

Confirm that the plugin functions in the admin, and that you can still create/edit segments and prompts as expected.
Confirm that the plugin functions on the front-end. Note that segmentation will be broken as noted above, but that you should see the new criteria-based segment matching tests being logged in the JS console as implemented in feat: segmentation criteria system #1155.
Confirm there are no new PHP fatals or warnings, and no new JS errors resulting from the deleted files.

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?

miguelpeixe · 2023-07-14T14:56:23Z

includes/class-newspack-popups-newsletters.php


-		if ( ! empty( $events_to_add ) ) {
-			$api->save_reader_events( $client_id, $events_to_add );
+			// TODO: set donation status in the Reader Data store.


That's been done in Automattic/newspack-plugin#2539

Ah, you're right—this file only checks subscription/donation status upon user login, but that's already handled by Automattic/newspack-plugin#2539. 🙌

Removed this file in 6c97c16

miguelpeixe · 2023-07-14T15:00:00Z

includes/class-newspack-popups-newsletters.php

+				],
+			];
+
+			// TODO: set subscription status in the Reader Data store.


Also resolved in Automattic/newspack-plugin#2539

You're right, thanks for pointing it out!

Removed this file in 6c97c16

miguelpeixe

What a beautiful changeset ❤️

I think we can also remove includes/class-newspack-popups-donations.php and includes/class-newspack-popups-newsletters.php, wdyt?

dkoo · 2023-07-14T17:14:55Z

I think we can also remove includes/class-newspack-popups-donations.php and includes/class-newspack-popups-newsletters.php, wdyt?

Removed those files in 6c97c16. You're right that they're no longer needed since the new reader data library API handles the newsletter/donation status updates on reader login.

dkoo · 2023-07-14T17:53:23Z

Realized there were still a couple of failing PHPunit tests that will no longer be needed (since the decision on which segments match the reader will no longer be executed in PHP). Removed in c08b6bc.

miguelpeixe

Nice! LGTM!

* 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 <leogermani@automattic.com> * 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 <miguel.peixe@a8c.com> Co-authored-by: Leo Germani <leogermani@automattic.com> * 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 <miguel.peixe@a8c.com> * 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 <leogermani@automattic.com> Co-authored-by: dkoo <derrick.koo@10up.com> * 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 <leogermani@automattic.com> * fix: lines added by accident in last merge * feat: move migration methods (#1184) Co-authored-by: Derrick Koo <derrick.koo@automattic.com> * 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 <miguel.peixe@automattic.com> Co-authored-by: Leo Germani <leogermani@automattic.com> Co-authored-by: Miguel Peixe <miguel.peixe@a8c.com> Co-authored-by: dkoo <derrick.koo@10up.com>

# [2.23.0-alpha.1](v2.22.0...v2.23.0-alpha.1) (2023-08-17) ### Bug Fixes * move the close button when Hide Padding is enabled ([#1165](#1165)) ([20d17d9](20d17d9)) ### Features * campaigns segmentation API rearchitecture ([#1192](#1192)) ([d554151](d554151)), closes [#1155](#1155) [#1166](#1166) [#1167](#1167) [#1159](#1159) [#1175](#1175) [#1169](#1169)

# [2.23.0](v2.22.0...v2.23.0) (2023-08-24) ### Bug Fixes * account for frequency presets in printed config ([#1203](#1203)) ([a97bc8f](a97bc8f)) * add .hidden styles for above header prompts ([#1207](#1207)) ([a19d64d](a19d64d)) * ensure url param segment is set ([#1197](#1197)) ([dc6c130](dc6c130)) * eslint new rule declaration-block-no-redundant-longhand-properties ([d1bf215](d1bf215)) * include missing file ([#1194](#1194)) ([d5fe344](d5fe344)) * move the close button when Hide Padding is enabled ([#1165](#1165)) ([20d17d9](20d17d9)) * remove string from overlay prompt ([#1204](#1204)) ([232f05a](232f05a)) ### Features * add segmentation README and more useful info to debug object ([#1195](#1195)) ([b501d9a](b501d9a)) * campaigns segmentation API rearchitecture ([#1192](#1192)) ([d554151](d554151)), closes [#1155](#1155) [#1166](#1166) [#1167](#1167) [#1159](#1159) [#1175](#1175) [#1169](#1169)

dkoo added 3 commits July 13, 2023 17:09

chore: remove Lightweight API and client ID handling

dbd975e

chore: remove data pruning cron job and clear future instances

305f79c

fix: script dependencies

03d0ced

dkoo added [Status] Needs Review refactor labels Jul 13, 2023

dkoo requested a review from a team as a code owner July 13, 2023 23:40

dkoo self-assigned this Jul 13, 2023

dkoo added 2 commits July 13, 2023 17:43

fix: remove unused method refs

f0254be

test: remove tests using deprecated features

06c8248

miguelpeixe reviewed Jul 14, 2023

View reviewed changes

miguelpeixe requested changes Jul 14, 2023

View reviewed changes

miguelpeixe requested a review from leogermani July 14, 2023 15:23

chore: remove more unneeded files

6c97c16

dkoo requested a review from miguelpeixe July 14, 2023 17:15

test: remove other failing tests

c08b6bc

miguelpeixe approved these changes Jul 14, 2023

View reviewed changes

github-actions bot added [Status] Approved and removed [Status] Needs Review labels Jul 14, 2023

dkoo merged commit 8a7bee5 into epic/rearchitecture Jul 14, 2023
1 check passed

dkoo deleted the remove/lightweight-api branch July 14, 2023 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove Lightweight API and client ID handling #1166

chore: remove Lightweight API and client ID handling #1166

dkoo commented Jul 13, 2023 •

edited

Loading

miguelpeixe Jul 14, 2023

dkoo Jul 14, 2023

dkoo Jul 14, 2023

miguelpeixe Jul 14, 2023

dkoo Jul 14, 2023

dkoo Jul 14, 2023

miguelpeixe left a comment

dkoo commented Jul 14, 2023

dkoo commented Jul 14, 2023

miguelpeixe left a comment

chore: remove Lightweight API and client ID handling #1166

chore: remove Lightweight API and client ID handling #1166

Conversation

dkoo commented Jul 13, 2023 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

miguelpeixe Jul 14, 2023

Choose a reason for hiding this comment

dkoo Jul 14, 2023

Choose a reason for hiding this comment

dkoo Jul 14, 2023

Choose a reason for hiding this comment

miguelpeixe Jul 14, 2023

Choose a reason for hiding this comment

dkoo Jul 14, 2023

Choose a reason for hiding this comment

dkoo Jul 14, 2023

Choose a reason for hiding this comment

miguelpeixe left a comment

Choose a reason for hiding this comment

dkoo commented Jul 14, 2023

dkoo commented Jul 14, 2023

miguelpeixe left a comment

Choose a reason for hiding this comment

dkoo commented Jul 13, 2023 •

edited

Loading