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

feat: track custom analytics events #243

Merged
merged 12 commits into from
Feb 29, 2024
Merged

feat: track custom analytics events #243

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9e91977
project reinstall
OchiengPaul442 Feb 25, 2024
ee0b944
Used google analytics to track the video playback and resource view b…
OchiengPaul442 Feb 25, 2024
c40ca9c
removed analytics from video-player
OchiengPaul442 Feb 25, 2024
aca790b
tracking video playback using google analytics
OchiengPaul442 Feb 25, 2024
f9e8301
eslint issue
OchiengPaul442 Feb 25, 2024
ab06048
eslint issue unresolved
OchiengPaul442 Feb 25, 2024
1bc6b5c
url change on video player component
OchiengPaul442 Feb 27, 2024
8b82c23
worked on setting ids for google analytics for the different events
OchiengPaul442 Feb 27, 2024
a80d87c
Merge branch 'main' into ft-custom-analytics
OchiengPaul442 Feb 27, 2024
eb39201
removed un used code
OchiengPaul442 Feb 27, 2024
6cf110b
Merge branch 'main' into ft-custom-analytics
OchiengPaul442 Feb 29, 2024
b55a77f
chore: review tidying
chrismclarke Feb 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion apps/picsa-tools/resources-tool/src/app/services/resources-tool.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Capacitor } from '@capacitor/core';
import { ConfigurationService } from '@picsa/configuration/src';
import { APP_VERSION } from '@picsa/environments/src';
import { PicsaAsyncService } from '@picsa/shared/services/asyncService.service';
import { AnalyticsService } from '@picsa/shared/services/core/analytics.service';
import { PicsaDatabase_V2_Service, PicsaDatabaseAttachmentService } from '@picsa/shared/services/core/db_v2';
import { FileService } from '@picsa/shared/services/core/file.service';
import { NativeStorageService } from '@picsa/shared/services/native';
Expand All @@ -21,7 +22,8 @@ export class ResourcesToolService extends PicsaAsyncService {
private dbAttachmentService: PicsaDatabaseAttachmentService,
private configurationService: ConfigurationService,
private nativeStorageService: NativeStorageService,
private fileService: FileService
private fileService: FileService,
private analyticsService: AnalyticsService
) {
super();
}
Expand Down Expand Up @@ -71,6 +73,8 @@ export class ResourcesToolService extends PicsaAsyncService {
}

public async openFileResource(uri: string, mimetype: string) {
// track the resource open event
this.analyticsService.trackResourceOpen();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit(blocking): Nice we can track the resource open event but would be even better if we could track the id of the resource being opened. So in order to achieve this we either need to pass an extra argument to the openFileResource method (e.g. id), or call the tracking function within the resource-item-file component itself which does have the id.

I think technically the better solution would probably be passing the id to the service, as that keeps the component more focused on the UI and service on side-effects

if (Capacitor.isNativePlatform()) {
try {
this.nativeStorageService.openFileURI(uri, mimetype);
Expand Down
5 changes: 4 additions & 1 deletion libs/shared/src/features/video-player/video-player.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Component, ElementRef, HostBinding, Input, OnDestroy } from '@angular/core';
import { Capacitor } from '@capacitor/core';
import { ScreenOrientation } from '@capacitor/screen-orientation';
import { AnalyticsService } from '@picsa/shared/services/core/analytics.service';
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit(blocking) - I see the tests are failing and they refer to this import in the errors (not sure if you have seen).
This is due to a convention with nx monorepos, that you shouldn't use package name prefixes when referring to files that area actually in the same package. As the analytics service and video-player component both sit within the shared package, you should change the import to be a relative one instead

import { AnalyticsService } from '../../services/core/analytics.service';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @chrismclarke, I noticed the build errors earlier and was working on resolving them. Your explanation has provided clarity on the issue, and I’m now in a better position to address it. Thanks

import { CapacitorVideoPlayer, CapacitorVideoPlayerPlugin, capVideoPlayerOptions } from 'capacitor-video-player';

// Fix listeners missing from type
Expand Down Expand Up @@ -53,7 +54,7 @@ export class VideoPlayerComponent implements OnDestroy {

private pauseTime: number = 0;

constructor(private elementRef: ElementRef<HTMLDivElement>) {}
constructor(private elementRef: ElementRef<HTMLDivElement>, private analyticsService: AnalyticsService) {}

async ngOnDestroy() {
await this.videoPlayer.stopAllPlayers();
Expand All @@ -74,6 +75,8 @@ export class VideoPlayerComponent implements OnDestroy {
if (Capacitor.isNativePlatform()) {
this.initialised = false;
}
// Track video play event
this.analyticsService.trackVideoPlay(this.playerId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit(non-blocking): currently the playerId is a randomly generated id, so it doesn't really hold any analytics value. It might be good to implement player IDs as suggested in #241 (but not the full storage solution) so that we can track the id of the resource when passed to the video player and for logging in analytics

This could either be included in this pr (would only be a few lines of code), or as part of a follow-up

// Initialise player any time playback triggered in case url updated (e.g. downloaded after init)
await this.initPlayer();
this.videoPlayer.play({ playerId: this.playerId }).then(() => {
Expand Down
20 changes: 18 additions & 2 deletions libs/shared/src/services/core/analytics.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { Injectable } from '@angular/core';
import { NavigationEnd,Router } from '@angular/router';
import { NavigationEnd, Router } from '@angular/router';
import { Capacitor } from '@capacitor/core';
import { FirebaseAnalytics } from '@capacitor-community/firebase-analytics';
import { APP_VERSION,ENVIRONMENT } from '@picsa/environments';
import { APP_VERSION, ENVIRONMENT } from '@picsa/environments';

@Injectable({ providedIn: 'root' })
/**
Expand Down Expand Up @@ -35,4 +35,20 @@ export class AnalyticsService {
}
});
}

// Method to track when users play a video
public trackVideoPlay(videoId: string) {
this.firebaseAnalytics.logEvent({
name: 'video_play',
params: { video_id: videoId, app_version: APP_VERSION },
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit(non-blocking): app_version will automatically be tracked in various other places so no need to explictily send with each analytics request, but very nice to have the video_id getting passed.

});
}

// Method to track when users opens resource file
public trackResourceOpen() {
this.firebaseAnalytics.logEvent({
name: 'open_resource_file',
params: { app_version: APP_VERSION },
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit(non-blocking): same as above, although would also be nice to have the resource_id tracked as part of the the payload sent to analytics so that we can track research questions such as which resources are accessed most frequently

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will work on that.

});
}
}
2 changes: 1 addition & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15147,7 +15147,7 @@ __metadata:
"leaflet-draw@github:enketo/Leaflet.draw#ff730785db7fcccbf2485ffcf4dffe1238a7c617":
version: 1.0.4
resolution: "leaflet-draw@https://github.com/enketo/Leaflet.draw.git#commit=ff730785db7fcccbf2485ffcf4dffe1238a7c617"
checksum: b08b88994769667f11f2b6a8937656c89cea34dafd4661abab0b48b4b97f3bddbdce7b23ddfdb8d7c6335e065530e32a70e281314afa34afa134bf68597945fc
checksum: ff548d48c7562be501934f81303061204450e037005ef08591049951adb3044778a6e9c97daa6bac57827ce8864d4637046a8a5669fab5ab43feec7bb3bed86f
languageName: node
linkType: hard

Expand Down
Loading