Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Google analytics #2534

Merged
merged 13 commits into from
Jun 2, 2020
Merged

Google analytics #2534

merged 13 commits into from
Jun 2, 2020

Conversation

raycarrick-ed
Copy link
Contributor

Fixes #2344 .

Changes proposed in this PR:

  • add google analytics tracker to org edit page
  • configure root org in application.rb
  • if root org has a tracker defined, plant it on every page
  • if current user org has a tracker defined plant that too

Copy link
Contributor

@briri briri left a comment

Choose a reason for hiding this comment

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

Looks good Ray. Just a few minor suggestions. Feel free to merge yourself afterward.

@@ -122,6 +122,9 @@ class Application < Rails::Application
config.branding = config_for(:branding).deep_symbolize_keys
end

# org abbreviation for the root google analytics tracker that gets planted on every page
config.tracker_root = "UoE"
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want something more generic here like 'DMPRoadmap' so that other installations aren't using UoE by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agh! that wasn't intentional

@@ -64,6 +64,10 @@ class Org < ActiveRecord::Base

belongs_to :region

has_one :tracker
accepts_nested_attributes_for :tracker
validates_associated :tracker
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I did not know about this validates_associated.

can you add a dependent: :destroy' to the has_one` statement? This should prevent any orphaning of records

require 'rails_helper'

RSpec.describe Tracker, type: :model do
pending "add some examples to (or delete) #{__FILE__}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great to add some simple tests here. Check out some of the other model tests like spec/models/contributor_spec.rb for examples of association and validation tests.
You can test that enum with something like this: it { is_expected.to define_enum_for(:category).with(Identifier.categories.keys) }

@raycarrick-ed raycarrick-ed merged commit 9248240 into development Jun 2, 2020
@raycarrick-ed raycarrick-ed deleted the google_analytics branch June 2, 2020 12:35
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants