Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add domain name support #1

Merged
merged 1 commit into from
Feb 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions app/models/application_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,4 @@ class ApplicationRecord < ActiveRecord::Base
self.implicit_order_column = :created_at

alias_attribute :_id, :id

def all_tags=(names)
Copy link
Owner Author

Choose a reason for hiding this comment

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

This was only ever a placeholder method, and was a less-than-ideal location for it, even then.

The new equivalent is better on both counts, I think.

tag_set = []
names.map do |name|
tag_set << Tag.where(name: name.strip).first_or_create!
end
self.tags = tag_set
end
end
8 changes: 0 additions & 8 deletions app/models/concerns/searchable_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,6 @@ def self.other_attributes(attributes = [])
end
end

def self.other_attributes(attributes = [])
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is also declared above, so this copy is redundant and is being removed. 👍

giphy-2

singleton_class.instance_eval do
define_method(:other_search_fields) do
attributes
end
end
end

# Add default accessors
SEARCHABLE_FIELD_GROUPS.each do |field_group|
next if respond_to? field_group
Expand Down
6 changes: 6 additions & 0 deletions app/models/domain_name.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true

class DomainName < ActiveRecord::Base
has_many :domain_registrations, dependent: :destroy
has_many :organizations, through: :domain_registrations
Copy link
Owner Author

Choose a reason for hiding this comment

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

Obviously, this approach would be extensible to other models by making it polymorphic, as we're already doing with the Tag model.

end
6 changes: 6 additions & 0 deletions app/models/domain_registration.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true

class DomainRegistration < ActiveRecord::Base
belongs_to :domain_name
belongs_to :organization
end
8 changes: 4 additions & 4 deletions app/models/organization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class Organization < ApplicationRecord
include SearchableConcern
boolean_attributes [:shared_tickets]
fulltext_attributes %i[url external_id name details]
join_attributes %i[tags]
join_attributes %i[domain_names tags]

include UuidConcern
uuid_attributes [:external_id]
Expand All @@ -15,9 +15,9 @@ class Organization < ApplicationRecord
has_many :taggings, as: :taggable, dependent: :destroy
has_many :tags, through: :taggings

has_many :domain_registrations, dependent: :destroy
has_many :domain_names, through: :domain_registrations

# Disable single-table inheritance
self.inheritance_column = :ignore_the_type_column

def domain_names=(_placeholder)
end
end
1 change: 0 additions & 1 deletion app/models/tagging.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
# frozen_string_literal: true


class Tagging < ActiveRecord::Base
belongs_to :tag
belongs_to :taggable, polymorphic: true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ def up
t.text :description
t.integer :priority, limit: 2, null: false, default: 3
t.integer :status, limit: 2, null: false, default: 1
t.references :submitter, foreign_key: { to_table: :users }, null: false
t.references :assignee, foreign_key: { to_table: :users }
t.references :organization, foreign_key: true
t.references :submitter, foreign_key: false, null: false
t.references :assignee, foreign_key: false
t.references :organization, foreign_key: false
t.boolean :has_incidents, null: false, default: false
t.datetime :due_at
t.integer :via, limit: 2, null: false, default: 1
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# frozen_string_literal: true

class CreateDomainNamesAndDomainRegistrations < ActiveRecord::Migration[6.0]
def change
create_table :domain_names do |t|
t.string :name, null: false
Copy link
Owner Author

Choose a reason for hiding this comment

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

For the purposes of this exercise, I've decided that all the join models will be searchable based on the name attribute - it's not an assumption that is likely to hold up outside of the context of an exercise like this, but for now at least, it felt like a reasonable approach.

t.timestamps null: false
end

create_table :domain_registrations do |t|
t.references :domain_name, foreign_key: true
t.references :organization, foreign_key: true
t.timestamps null: false
end
end
end
22 changes: 18 additions & 4 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,22 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2020_02_17_014028) do
ActiveRecord::Schema.define(version: 2020_02_17_224235) do

create_table "domain_names", force: :cascade do |t|
t.string "name", null: false
t.datetime "created_at", precision: 6, null: false
t.datetime "updated_at", precision: 6, null: false
end

create_table "domain_registrations", force: :cascade do |t|
t.integer "domain_name_id"
t.integer "organization_id"
t.datetime "created_at", precision: 6, null: false
t.datetime "updated_at", precision: 6, null: false
t.index ["domain_name_id"], name: "index_domain_registrations_on_domain_name_id"
t.index ["organization_id"], name: "index_domain_registrations_on_organization_id"
end

create_table "organizations", force: :cascade do |t|
t.string "url"
Expand Down Expand Up @@ -83,9 +98,8 @@
t.index ["organization_id"], name: "index_users_on_organization_id"
end

add_foreign_key "domain_registrations", "domain_names"
add_foreign_key "domain_registrations", "organizations"
add_foreign_key "taggings", "tags"
add_foreign_key "tickets", "organizations"
add_foreign_key "tickets", "users", column: "assignee_id"
add_foreign_key "tickets", "users", column: "submitter_id"
add_foreign_key "users", "organizations"
end
18 changes: 16 additions & 2 deletions db/seeds.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@

IMPORT_ENTITIES = [Organization, User, Ticket].freeze

def names_to_models(entity, array)
models = []
array.map do |name|
models << entity.where(name: name.strip).first_or_create!
end
models
end

IMPORT_ENTITIES.each do |entity|

if entity.count == 0
Expand All @@ -10,8 +18,14 @@
records = JSON.parse(File.read(path))

records.each do |record|
record[:all_tags] = record.delete('tags')
entity.create!(record)
tags = record.delete('tags')
domain_names = record.delete('domain_names')

record[:tags] = names_to_models(Tag, tags) if tags.present?
record[:domain_names] = names_to_models(DomainName, domain_names) if domain_names.present?

model = entity.new(record)
model.save!(validate: false)
print '.'
end

Expand Down
6 changes: 3 additions & 3 deletions db/seeds/tickets.json
Original file line number Diff line number Diff line change
Expand Up @@ -1765,7 +1765,7 @@
"description": "Duis mollit Lorem amet anim ea consequat ullamco. Eiusmod tempor incididunt adipisicing in mollit ut officia eu sint sit amet aute.",
"priority": "normal",
"status": "closed",
"submitter_id": 55,
"submitter_id": 555,
Copy link
Owner Author

Choose a reason for hiding this comment

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

As noted in 7ccbc59 - several of these ticket records have what appears to be invalid data.

I assume that's intentional, and part of the exercise. I'd initially set these to valid values just to defer a decision on how to handle things.

I'm not super enthusiastic about losing our referential integrity, but it feels less-bad for our context here than excluding records.

Reviewers, if you're reading this (👋) - definitely interested in hearing opinions on preferred tradeoffs here.

"assignee_id": 17,
"organization_id": 114,
"tags": [
Expand Down Expand Up @@ -1927,7 +1927,7 @@
"status": "closed",
"submitter_id": 20,
"assignee_id": 33,
"organization_id": 101,
"organization_id": 555,
"tags": [
"South Dakota",
"Montana",
Expand Down Expand Up @@ -4387,7 +4387,7 @@
"priority": "urgent",
"status": "open",
"submitter_id": 13,
"assignee_id": 55,
"assignee_id": 555,
"organization_id": 108,
"tags": [
"Minnesota",
Expand Down
10 changes: 10 additions & 0 deletions test/services/search_entities/organization_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,14 @@ def do_setup

assert_includes search.results, tag_org, 'Should find organizations by tag'
end

test 'finds_by_domains' do
domain_name = 'brawndo.com'
domain_org = create_organization!
domain_org.domain_names << DomainName.where(name: domain_name).first_or_create!
search_params = { domain_names: domain_name }
search = SearchEntities.call(Organization, search_params)

assert_includes search.results, domain_org, 'Should find organizations by tag'
Copy link
Owner Author

Choose a reason for hiding this comment

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

This error message should say "by domain name", but given that refactoring how we are constructing scopes is part of my next set of changes, I'll roll that update into that commit.

end
end
25 changes: 24 additions & 1 deletion test/services/search_entities/user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,16 @@ def do_setup
)
end

test 'does_not_find_empty_text_when_unset' do
blank_user = create_user!(signature: '')
nil_user = create_user!(signature: nil)
search_params = {}
search = SearchEntities.call(User, search_params)

refute_includes search.results, blank_user, 'Should not find users by empty text when text is not a parameter'
refute_includes search.results, nil_user, 'Should not find users by nil text when text is not a parameter'
end

test 'finds_by_boolean_fields' do
search_params = { active: 'false' }
search = SearchEntities.call(User, search_params)
Expand All @@ -79,12 +89,25 @@ def do_setup
)
end

test 'finds_by_other_fields' do
danish_user = create_user!(locale: 'da-DK')
search_params = { locale: 'da-DK' }
search = SearchEntities.call(User, search_params)

assert_includes search.results, danish_user, 'Should find users by other value'

assert(
[@url_user, @external_id_user, @name_user, @signature_user].all? do |org|
search.results.exclude? org
end, 'Should not find users without matching other values'
)
end

test 'finds_by_combined_fields' do
search_params = { text: 'sit', active: 'false' }
search = SearchEntities.call(User, search_params)

assert_includes search.results, @name_user, 'Should find users by text in shared context'
assert_includes search.results, @inactive_user, 'Should find users by boolean in shared context'
end

end