-
Notifications
You must be signed in to change notification settings - Fork 230
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
WSTEAMA-1412: Add Reverb dependency as the transpiled version #12189
Merged
Merged
Changes from 80 commits
Commits
Show all changes
100 commits
Select commit
Hold shift + click to select a range
1af2a31
Add reverb as a dependency
karinathomasbbc 881309c
use git+ssh syntax
karinathomasbbc d8d342b
Following guidelines in https://github.com/bbc/delivery-engineering/t…
karinathomasbbc 50f1608
Following guidelines in https://github.com/bbc/delivery-engineering/t…
karinathomasbbc ecf0f23
TEMP: don't check package manager; remove winston as npm does not sup…
karinathomasbbc 8cf70ff
Adding GITHUB_TOKEN to builds which install node modules
karinathomasbbc f4eb63c
Use git+ssh syntax
karinathomasbbc d7c72fb
Revert "TEMP: don't check package manager; remove winston as npm does…
karinathomasbbc 5dcb396
Use github instead of github+ssh syntax
karinathomasbbc 2fe6d28
Update bbc/reverb reference
alex-magana edbba63
Change reference
alex-magana 21134ea
Add flag
alex-magana bdc6cba
Revert flag
alex-magana e54735f
Change reverb reference
alex-magana a56a4f3
Remove Reverb from cache and lock file
alex-magana 5617923
Update Reverb reference to match Webcore
alex-magana a7cfef2
Serve compiled Reverb code
alex-magana 164e389
Use correct reference for smart tag
alex-magana 443d632
Add reverb loading boilerplate code
alex-magana 0bce537
Disable withReverb in applyBasicPageHandlers
alex-magana f64a497
Fire page view event via Reverb
alex-magana 76f28ad
Use static Reverb in sendBeacon module
alex-magana ea302c5
Add click tracking for MostRead
alex-magana d5ff79d
Remove sample Reverb data
alex-magana b144379
Add view tracking support
alex-magana 5529207
Enable Top Stories view tracking
alex-magana dba7875
Add click tracking for Top Stories
alex-magana d91a4fc
Enable click and view tracking for Feature Analysis component
alex-magana fd66f0c
Revert setting GITHUB_TOKEN in workflow config
alex-magana 75fd19e
Remove reverb from package.json
alex-magana 06460e8
Remove local compiled versions of reverb in favour of mybbc-analytics…
alex-magana 84aa432
Delete the withReverb page handler
alex-magana 5e20221
Invoke event functions if reverb is ready
alex-magana 4d15f30
Fix return linting error
alex-magana 068f6b0
Make the reverb static script non-blocking
alex-magana 1d1e1ba
To be reverted - make atiAnalyticsProducerName optional
alex-magana 736a246
Merge pull request #12226 from bbc/WSTEAMA-1482-optimise-reverb-initi…
alex-magana ca43537
Merge branch 'latest' into reverb-dependency-minified
alex-magana 6f86298
Merge branch 'latest' into reverb-dependency-minified
alex-magana 47cde59
adds producer name value for afaanoromoo
louisearchibald 109b613
correct name
louisearchibald 974f998
adds producer name value for afrique amharic arabic archive and azeri
louisearchibald 71b8703
add producer name to config for bengali burmese cymrufyw gahuza gujar…
louisearchibald b3a2931
update config for hindi igbo indonesia japanese and korean
louisearchibald 91e38e1
update kyrgyz marathi mundo naidheachdan & nepali
louisearchibald 99570ed
update config for news newsround pashto pidgin & portuguese
louisearchibald 721cc0c
update service config for punjabi russian scotland serbian & sinhala
louisearchibald 92addb7
update service config for somali sport swahili tamil & telugu
louisearchibald 078e69f
update service config for thai tigrinya turkce ukchina & ukrainian
louisearchibald c18fb40
update service config for urdu uzbek vietnamese ws yoruba & zhongwen
louisearchibald a45c784
Remove useReverb prop from ATIAnalytics component
alex-magana f0c28b2
Remove useReverb prop from the MostRead component
alex-magana 3e46f44
Remove useReverb prop from Top Stories component
alex-magana 5a008d4
Remove useReverb prop from the FeaturesAnalysis component
alex-magana 21122ae
Merge branch 'latest' into reverb-dependency-minified
alex-magana 1c68e74
Merge pull request #12244 from bbc/WSTEAMA-1508-adds-producerName-to-…
alex-magana d98957c
Merge pull request #12253 from bbc/WSTEAMA-1496-enable-reverb-service…
alex-magana 7e19f85
Add producerName to config generation assertions
alex-magana f68a76b
Fix ATIAnalytics and EventTrackingContext ATI tests assertions
alex-magana 37f57da
Update PageLayoutWrapper copyright text
alex-magana 547e8f7
Add Reverb boilerplate to the CanonicalRenderer snapshot
alex-magana f5ea110
Add producer name to snapshot
alex-magana 55035ad
Merge branch 'latest' into reverb-dependency-minified
alex-magana 28465d9
Merge branch 'latest' into reverb-dependency-minified
eagerterrier 9e013d7
Update snapshots to reflect 2025 in the page copyright
alex-magana 78e8dfb
Merge branch 'reverb-dependency-minified' of github.com:bbc/simorgh i…
alex-magana 4216516
WSTEAMA-1484 - Reverb unit tests (#12265)
amoore108 5fa3c20
Add custom resource loader to fetch Reverb
alex-magana d28acca
Merge branch 'reverb-dependency-minified' into add-custom-resource-lo…
alex-magana 9bc880f
Merge pull request #12267 from bbc/add-custom-resource-loader
alex-magana 65793a4
Merge branch 'latest' into reverb-dependency-minified
alex-magana b2885fc
Merge branch 'latest' into reverb-dependency-minified
alex-magana 829d834
Fix passing event tracking data blocks for FeaturesAnalysis
alex-magana 5919981
Merge branch 'latest' into reverb-dependency-minified
alex-magana 6b4279c
Merge branch 'latest' into reverb-dependency-minified
pvaliani 608962d
Use dynamic values for click tracking
alex-magana 648b225
Merge branch 'latest' into reverb-dependency-minified
alex-magana 34ab7c1
Update tests to assert dynamic tracking values are passed
alex-magana cc6a98e
Merge branch 'reverb-dependency-minified' of github.com:bbc/simorgh i…
alex-magana 8e37d32
Merge branch 'latest' into reverb-dependency-minified
amoore108 f70f11a
Move the Reverb URL to env config
alex-magana 4dcaa04
Merge branch 'reverb-dependency-minified' of github.com:bbc/simorgh i…
alex-magana 3d96206
Set isSignedIn to false
alex-magana 3d0ed10
Merge branch 'latest' into reverb-dependency-minified
alex-magana 331aa9f
Remove hashedId assignment
alex-magana 07fc07b
Merge branch 'reverb-dependency-minified' of github.com:bbc/simorgh i…
alex-magana dec4da6
Merge branch 'latest' into reverb-dependency-minified
alex-magana d93b087
Assert use of environment variables persisted in the .env file
alex-magana caa630b
Merge branch 'latest' into reverb-dependency-minified
alex-magana 33e1368
Merge branch 'reverb-dependency-minified' of github.com:bbc/simorgh i…
alex-magana 3107f7f
Fix typo
alex-magana a040eaa
Merge branch 'latest' into reverb-dependency-minified
alex-magana 99ea38d
Merge branch 'latest' into reverb-dependency-minified
alex-magana e5fd7a3
Merge branch 'latest' into reverb-dependency-minified
alex-magana a032940
Merge branch 'latest' into reverb-dependency-minified
alex-magana c6fc194
Merge branch 'latest' into reverb-dependency-minified
alex-magana d6453b2
Merge branch 'latest' into reverb-dependency-minified
alex-magana f27f8e4
Merge branch 'latest' into reverb-dependency-minified
alex-magana 85c6503
Merge branch 'latest' into reverb-dependency-minified
alex-magana ff9fa82
Merge branch 'latest' into reverb-dependency-minified
alex-magana File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the same logic as above (lines 480-481)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for highlighting this Karina.
I reckon we should. Let me take a look at the existing logic
to confirm what informed this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've set this to
false
in both buildReverbAnalyticsModel and buildReverbPageSectionEventModel.This is informed by the documentation here.
I've checked the beacons and confirmed that
idclient
is still reported in beaconsfired to ATI/Piano.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I was working on my personalisation project, I had set it to be
!!hashedId
- see #11353 (comment)@gavinspence is this something we would like to put in place now, or shall we leave it for later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context. I've responded in the thread in 11353 for continuity.
I'll hold off merging pending steer from @gavinspence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From discussing with @gavinspence, we've concluded that we should retain
isSignedIn
asfalse
to:ensures that when personalisation is introduced, the aggregated sign-in data won't
include data from a time when we reported visitors as being signed in even though they were not.
While investigating this, we've established that Reverb doesn't use
hashedId
to trackvisitors. This property is used for accounts/users contrary to our initial understanding.
From the Reverb code, it appears Reverb logic doesn't use the cookie,
atuserid
,to persist visitor IDs. We're awaiting clarification on how to pass in the
idclient
value.Based on these findings, we may not need the
user
object in the Reverb config after all.In summation, since visitors are identified using the
idclient
value obtained fromatuserid
(or the equivalent cookie in Reverb), I believe we should be able to correlate visitors and accounts if/when they are created since we'll begin to pass in thehashedId
which we'll then be able to cross-reference against existingidclient
UUIDs.Please let me know if this position works for you @karinathomasbbc , in which case we can proceed
to merge the PR and fix forward the
idclient
configuration once we've heard from Reverb.With this merged, we should then be able to enable Reverb on TEST for Pidgin and examine the
tracking beacons further for any needed improvements prior to going LIVE for the low impact trial.
For future reference:
https://support.piano.io/hc/en-us/articles/4468178673170-Concept-of-Visitors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine @alex-magana, thanks for the detailed response and for following up with Product.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brill! I'll proceed to merge this.
Thanks Karina.