CarnegieLearningWeb / UpGrade

Framework for adding A/B testing to education applications
https://www.upgradeplatform.org/
BSD 3-Clause "New" or "Revised" License
26 stars 13 forks source link

Metrics not handled correctly by initMetrics.ts #1884

Open Innoviox opened 2 months ago

Innoviox commented 2 months ago

Version where bug was found: dev latest (54de016)

Describe the bug The current .env.docker.local.example has a different idea of what should be in METRICS than backend/packages/Upgrade/src/init/seed/initMetrics.ts. In the env, it is just a metrics array, but initMetrics expects an object containing a metrics and a contexts attribute. This causes MetricService.saveAllMetrics to be called with metrics=undefined, which instantly crashes the app.

To Reproduce Install the app:

  1. git clone https://github.com/CarnegieLearningWeb/UpGrade.git
  2. cd Upgrade
  3. cd frontend && npm install
  4. cd ../backend/packages/Upgrade && npm install
  5. cd ../../../ && mv ./backend/packages/Upgrade/.env.docker.local.example ./backend/packages/Upgrade/.env.docker.local
  6. docker-compose -f singleContainerApp-docker-compose.yml up -d

Expected behavior The backend starts up correctly.

Screenshots

image image

Desktop (please complete the following information):

Additional context To fix this, I believe the .env.docker.local.example should have an updated METRICS that reflects its actual usage in the application. Additionally, in backend/packages/Upgrade/src/api/services/MetricService.ts, line 168, this line could be added:

private parseMetrics(metrics: Array<IGroupMetric | ISingleMetric>): IMetricUnit[] {
    // add this line since this function gets called with undefined
    if (!metrics) return [];
    // ... rest of file

to prevent issues like this from completely crashing the backend.

ppratikcr7 commented 1 week ago

@Innoviox Thanks a lot for pointing this out. We will update this and create a PR soon.