Skip to content

Make the gem work without ActiveRecord#195

Merged
goodviber merged 7 commits intoDFE-Digital:mainfrom
hrmtl:make-activerecord-optional
Apr 23, 2025
Merged

Make the gem work without ActiveRecord#195
goodviber merged 7 commits intoDFE-Digital:mainfrom
hrmtl:make-activerecord-optional

Conversation

@hrmtl
Copy link
Copy Markdown
Contributor

@hrmtl hrmtl commented Apr 10, 2025

The dfe-analytics gem currently assumes the presence of a database and ActiveRecord in any app that uses it.

This PR adds a few modifications to allow the gem to work without a database.

The motivation for this came when we started to incorporate the gem into the GHBS FABS Rails app. Our app doesn't use a database. It uses Contentful CMS as its backend.

We only need the web requests tracking feature of dfe-analytics.

This PR makes the following modifications to allow the use of the gem without a database:

  • Check if ActiveRecord is defined before loading and running any dependent code.
  • Group together files which use ActiveRecord.
  • Update the initializer log message.
  • Add a test

As per my discussion with @asatwal, there may be scope to do a bigger refactor to modularise all the AR-related code. However, I did not want to make any big changes with my first PR seeing as many projects are using this gem.

This PR does just enough to make the gem usable without AR. Any feedback or suggestions are welcome.

Copy link
Copy Markdown
Collaborator

@asatwal asatwal left a comment

Choose a reason for hiding this comment

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

👍 Looks like a good change to not rely on ActiveRecord.

@hrmtl - Have you tested that this works in an environment that does not have ActiveRecord ?

@goodviber - Could you review also please and what's your opinion on having a config item also to control sending db events ?

return
end

if defined?(ActiveRecord)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We may want to introduce a config item, such as send_db_events to control this rather than just relying on ActiveRecord being present or not. TBD.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did try that but the gem code would throw an error while loading in the Rails app. I agree that a config item would be nice, but maybe we can do it as an improvement in the future.

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.

What was the error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I got a NameError for ActiveRecord when starting the Rails server. I think that happened when the gem code was loaded before the config got read.

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.

@asatwal I think we could look at the config issue later, I will have a tilt at it myself. If this has been tested @hrmtl with an app running ActiveRecord we can approve

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@goodviber Thanks. I haven't tested it with an app running ActiveRecord. Does the test suite not cover that?

@hrmtl
Copy link
Copy Markdown
Contributor Author

hrmtl commented Apr 15, 2025

@asatwal Yes, I've tested it in the FABS Rails app which doesn't have ActiveRecord. I was able to successfully track web requests into BigQuery.

@hrmtl
Copy link
Copy Markdown
Contributor Author

hrmtl commented Apr 15, 2025

@asatwal If you have CI setup here, can we run the test suite please?

Copy link
Copy Markdown
Collaborator

@asatwal asatwal left a comment

Choose a reason for hiding this comment

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

👍 - Nice work. Looks good to me.

@hrmtl
Copy link
Copy Markdown
Contributor Author

hrmtl commented Apr 22, 2025

@asatwal Thanks!

@goodviber goodviber merged commit 2a625c4 into DFE-Digital:main Apr 23, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants