Skip to content

Add extra required data to the track_event call when creating a school#899

Merged
jamiebenstead merged 6 commits into
mainfrom
1485-add-extra-data-to-school-created-event-tracking
Jun 30, 2026
Merged

Add extra required data to the track_event call when creating a school#899
jamiebenstead merged 6 commits into
mainfrom
1485-add-extra-data-to-school-created-event-tracking

Conversation

@jamiebenstead

Copy link
Copy Markdown
Contributor

Status

Description of what's been done

  • Add first_landing_page and marketing_preference data when saving the School - Created event

@cla-bot cla-bot Bot added the cla-signed label Jun 26, 2026
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown

Test coverage

92.05% line coverage reported by SimpleCov.
Run: https://github.com/RaspberryPiFoundation/editor-api/actions/runs/28438562270

'School - Created',
school_id: @school.id,
first_landing_page: params[:first_landing_page],
marketing_parameters: marketing_parameters

@zetter-rpf zetter-rpf Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it would be easier to query in the data warehouse if all of the event properties were at the same level. It might just be harder to document and query if some are nested inside other attributes. This means we could query on events.properties['utm_source'] rather than events.properties['marketing_params']['utm_source'].

You should be able to do this by spatting the marketing parameters:

track_event('School - Created', **marketing_parameters, @school.id:, first_landing_page: params[:first_landing_page])

@zetter-rpf zetter-rpf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great. added a suggestion - let me know if you think it's worth doing.

@jamiebenstead jamiebenstead marked this pull request as ready for review June 30, 2026 10:21
Copilot AI review requested due to automatic review settings June 30, 2026 10:21

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 390cb3a. Configure here.

Comment thread app/controllers/api/schools_controller.rb Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the school creation analytics event so additional attribution metadata is included when a school is created via the REST API.

Changes:

  • Expands the School - Created track_event call to include first_landing_page and flattened marketing parameters.
  • Adds a request spec asserting that marketing_parameters keys become top-level event properties (and the nested hash is not stored).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
spec/features/school/creating_a_school_spec.rb Adds coverage ensuring marketing parameters are recorded as top-level event properties.
app/controllers/api/schools_controller.rb Extends School - Created event tracking to include marketing attribution fields.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app/controllers/api/schools_controller.rb
Comment thread app/controllers/api/schools_controller.rb

@zetter-rpf zetter-rpf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The track_event call looks good
I saw the comments from cursor and copilot - do you think they could be problems?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread app/controllers/api/schools_controller.rb
Comment thread app/controllers/api/schools_controller.rb

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comment on lines +35 to +38
marketing_parameters.merge(
school_id: @school.id,
first_landing_page: params[:first_landing_page]
)
Comment on lines +33 to +39
track_event(
'School - Created',
marketing_parameters.merge(
school_id: @school.id,
first_landing_page: params[:first_landing_page]
)
)
Comment on lines +84 to +85

properties = Event.last.properties

@zetter-rpf zetter-rpf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice one

@jamiebenstead jamiebenstead merged commit fcd587f into main Jun 30, 2026
7 checks passed
@jamiebenstead jamiebenstead deleted the 1485-add-extra-data-to-school-created-event-tracking branch June 30, 2026 13:07
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.

3 participants