Fix for DfE::Analytics crashing on installation #93
Conversation
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
asatwal
previously approved these changes
Oct 17, 2023
asatwal
dismissed
their stale review
On second thoughts do we need this for Production ?
asatwal
reviewed
Oct 18, 2023
lib/dfe/analytics.rb
Outdated
Comment on lines
98
to
101
| unless File.exist?(Rails.root.join('config/initializers/dfe_analytics.rb')) | ||
| Rails.logger.info('Warning: DfE Analytics is not set up. Run `bundle exec rails generate dfe:analytics:install') | ||
| return | ||
| end |
There was a problem hiding this comment.
Just had a second thought on this. Is this required for Production ? And should we write to the terminal ?
Suggested change
| unless File.exist?(Rails.root.join('config/initializers/dfe_analytics.rb')) | |
| Rails.logger.info('Warning: DfE Analytics is not set up. Run `bundle exec rails generate dfe:analytics:install') | |
| return | |
| end | |
| if !Rails.env.production? && !File.exist?(Rails.root.join('config/initializers/dfe_analytics.rb')) | |
| message = "Warning: DfE Analytics is not set up. Run: 'bundle exec rails generate dfe:analytics:install'" | |
| Rails.logger.info(message) | |
| puts message | |
| return | |
| end |
There was a problem hiding this comment.
Yes, good plan!
ericaporter
commented
Oct 23, 2023
| unless Rails.env.production? || File.exist?(Rails.root.join('config/initializers/dfe_analytics.rb')) | ||
| message = "Warning: DfE Analytics is not set up. Run: 'bundle exec rails generate dfe:analytics:install'" | ||
| Rails.logger.info(message) | ||
| puts message | ||
| return | ||
| end |
There was a problem hiding this comment.
For my brain, unless || is easier to read but happy to update to if !
There was a problem hiding this comment.
Randomly saw this. First time I see unless || described as easier to read 😅. I personally avoid it.
There was a problem hiding this comment.
Ha, well, imo it follows the pattern with the other unless just above it, plus I think easier than if doesn't && doesn't @vassyz, but I shall bear that in mind next time I use unless
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Relating to this ticket: https://trello.com/c/ApoCu21a/1381-dfeanalytics-crashes-on-installation There is a bug where the gem crashes on installation
Investigation: Identified the release, PR and then commit which appears to be causing the problem 52486c5
It would seem that after adding
def self.initialize!andDfE::Analytics::Fields.check!the generators are not running first and the following error message could be observed:@asatwal, I have gone for adding a check in the initialization method, returning unless dfe_analytics.rb exists (actually if we go for this solution, it might be better to have a check for all the files the generator should create including the three .yml files).
I know there are a few ways we could tackle this one but this seemed like the most straightforward - keen to know if you think it will be sufficient.
Testing the fix: in ECF, in a demo app I spun up to investigate this issue, in Register and in Apply (all locally)
I uninstalled and installed the gem with my fix (pointing to my branch)
Interestingly the fix did not work in either Apply or Register, but that is because of some legacy config in application.rb (config.analytics = config_for(:analytics)) which predates the gem.
I don’t think this matters, it is only worth noting that if there is a service that has this config that tries to install the gem, it might still crash on installation, but removing this config would fix the problem.
Might be worth making apply and register aware of the legacy config so they can remove it.