diff --git a/README.md b/README.md index 61c53fb..2bb64e1 100644 --- a/README.md +++ b/README.md @@ -38,15 +38,15 @@ We use open-uri with Nokogiri for URL fetching and parsing ## Features -3. Friend setup -3. search -2. Node walking +## Robustness Concerns -## Robustness +1. Validation on Expert URL, existence, formatting, etc. Currently we just assume URL will be good, valid, and parsable. +1. Expert Topics: There is minor cleanup of topics. Need to review common words and phrases and filter out high frequency non-relevant ones, or whatever handling needs to be done. Switch from word list to method for better handling. +2. Expert Search: This is rudimentary by most standards. Postgres has built in full text search, but we thought that hid too much of the search algorithm (as per instructions). Ranked searching also is rudimentary, count up number of hits; but it could be enhanced maybe with a weight for the H1..H3 level in there. There are also engines like Elastisearch and Sphinx that handle the hard parts (and work with word stems, plurals, etc.) but those seemed to be prohibited by the instruction. All of which really says writing your own search from basic principals is "hard" to get it right, when you can leverage what the bigger players are doing. +3. Expert Path: We wrote our own, but there is database recursion in postgres and other ways to do a better more complete node path. Depending on scale, a graph database for the relationships might be better. +4. Friendship: We did a simple expert 1 to expert 2 model. In working on this we debated a two way model which has an expert and friend, and you would have to converse for your friend (a record I am an expert and you're my friend, and a record where you are the expert and I am the friend.). That solves some of the problem of searching, but introduces scaling issues of maintaining a consistent set of dual records for each edge between nodes, but it is worth considering. -1. Validation on Expert URL, existence, formatting, etc. - -## Scalability +## Scalability Issues 1. Save Expert does an inline URL grab and parse 1. Save Expert does an inline URL Shortening diff --git a/app/controllers/experts_controller.rb b/app/controllers/experts_controller.rb index 51311b5..ebaf054 100644 --- a/app/controllers/experts_controller.rb +++ b/app/controllers/experts_controller.rb @@ -34,7 +34,7 @@ def friends def add_friend Friendship.create(expert_1_id: @expert.id, expert_2_id: params[:friend_id]) - redirect_to @expert + redirect_to friends_expert_path(@expert) end def remove_friend @@ -42,11 +42,12 @@ def remove_friend if friendship friendship.destroy end - redirect_to @expert + redirect_to friends_expert_path(@expert) end def search_experts @topic_experts = @expert.topic_experts(params[:q]) + @search_term = params[:q] end private diff --git a/app/helpers/experts_helper.rb b/app/helpers/experts_helper.rb index 5b1d260..c6dad17 100644 --- a/app/helpers/experts_helper.rb +++ b/app/helpers/experts_helper.rb @@ -1,2 +1,9 @@ module ExpertsHelper + + def introduction_path(expert, target) + experts = expert.path_to_expert(target) + return "Sorry, no introduction path" unless experts + experts.map{|e| e.name}.join(" => ") + end + end diff --git a/app/models/expert.rb b/app/models/expert.rb index 5d6b771..e59f047 100644 --- a/app/models/expert.rb +++ b/app/models/expert.rb @@ -10,30 +10,40 @@ class Expert < ApplicationRecord after_validation :generate_short_url after_create :get_topics + UNDESIRED_CONTENT = ['more', 'view', 'search', 'contents'] + def get_topics page = Nokogiri::HTML(open(url)) page.css('h1').each do |element| content = element.content - next if content.blank? + next if content.blank? || UNDESIRED_CONTENT.include?(content.downcase) topics.create(tag: 'h1', content: content) end page.css('h2').each do |element| content = element.content - next if content.blank? + next if content.blank? || UNDESIRED_CONTENT.include?(content.downcase) topics.create(tag: 'h3', content: content) end page.css('h3').each do |element| content = element.content - next if content.blank? + next if content.blank? || UNDESIRED_CONTENT.include?(content.downcase) topics.create(tag: 'h3', content: content) end end def generate_short_url - bitly = Bitly.new(ENV["SHORTENER_USERNAME"],ENV["SHORTENER_API_KEY"]) - response = bitly.shorten(url) - self.short_url = response.short_url + if url.blank? + errors.add :url, "can't be blank." + else + begin + bitly = Bitly.new(ENV["SHORTENER_USERNAME"],ENV["SHORTENER_API_KEY"]) + response = bitly.shorten(url) + self.short_url = response.short_url + rescue BitlyError => e + errors.add :url, e.message + end + end end def friends @@ -41,8 +51,26 @@ def friends end def topic_experts(a_topic) - friend_ids = [11,12,13,1] - Topic.includes(:expert).where("content ilike ?", "%#{a_topic}%").where.not("experts.id": id).where.not("experts.id": friend_ids).map{|t| t.expert } + word_atoms = a_topic.split + friend_ids = Friendship.where(expert_1_id: id).map{|f| f.expert_2_id} + Friendship.where(expert_2_id: id).map{|f| f.expert_1_id} + matching_topics = Topic.includes(:expert).where.not("experts.id": id).where.not("experts.id": friend_ids) + word_atoms.each do |atom| + matching_topics = matching_topics.where("content ilike ?", "%#{atom}%") + end + matching_topics.map{|t| t.expert }.each_with_object(Hash.new(0)) {|expert, counts| counts[expert] += 1} end + # returns a path to the expert in the form of an array of experts, or false if the expert is unreachable + def path_to_expert(target, path=[]) + path << self + return path if target == self + results = [] + friends.each do |friend| + next if path.include?(friend) # been here before + new_path = friend.path_to_expert(target, path.clone) + results << new_path if new_path + end + return false if results.empty? # termination case + return results.sort_by(&:length).first # winner winner chicken dinner + end end diff --git a/app/models/friendship.rb b/app/models/friendship.rb index 7f11a6e..74ff599 100644 --- a/app/models/friendship.rb +++ b/app/models/friendship.rb @@ -3,5 +3,9 @@ class Friendship < ApplicationRecord belongs_to :expert_2, :class_name => "Expert", :foreign_key => "expert_2_id" validates :expert_1_id, :expert_2_id, presence: true + validate :no_identity_friendship + def no_identity_friendship + errors.add :expert_2, "can't be friends with themselves." if expert_1_id == expert_2_id + end end diff --git a/app/views/experts/friends.html.erb b/app/views/experts/friends.html.erb index 0d0ab75..2a85fff 100644 --- a/app/views/experts/friends.html.erb +++ b/app/views/experts/friends.html.erb @@ -3,9 +3,9 @@ <% @experts.each do |expert| %>
<% if @expert.friends.include?(expert) %> - <%= link_to "-", remove_friend_experts_path(@expert.id, expert.id), method: :delete %> + <%= link_to "-", remove_friend_experts_path(@expert.id, expert.id), method: :delete, class: 'btn btn-sm btn-primary' %> <% else %> - <%= link_to "+", add_friend_experts_path(@expert.id, expert.id), method: :post %> + <%= link_to "+", add_friend_experts_path(@expert.id, expert.id), method: :post, class: 'btn btn-sm btn-primary' %> <% end %> <%= expert.name %>
diff --git a/app/views/experts/search_experts.html.erb b/app/views/experts/search_experts.html.erb index 15bcc7d..e84152e 100644 --- a/app/views/experts/search_experts.html.erb +++ b/app/views/experts/search_experts.html.erb @@ -3,9 +3,26 @@

<%= @expert.name %>

-<% @topic_experts.each do |te| %> - <%- if current_user %> - <%= link_to "+", add_friend_experts_path(@expert.id, te.id), method: :post, class: 'btn btn-sm btn-primary' %> +

+ <%= form_for( + @expert, + url: search_experts_expert_path, + method: :get, + ) do |f| %> + Search for another Skill: <%= text_field_tag :q %> + <%= f.submit 'Search', class: 'btn btn-sm btn-primary' %> + <% end %> +

+ +<% if @topic_experts.blank? %> +

Sorry, no experts were found for "<%= @search_term %>"

+<% else %> +

Experts matching "<%= @search_term %>"

+ <% @topic_experts.sort_by{|k,v| v }.reverse.each do |te, value| %> + <%- if current_user %> + <%= link_to "+", add_friend_experts_path(@expert.id, te.id), method: :post, class: 'btn btn-sm btn-primary' %> + <% end %> + <%= link_to te.name, te %> (Ranking: <%= value %>)
+ <%= introduction_path @expert, te %>
<% end %> - <%= te.name %>
<% end %> diff --git a/app/views/experts/show.html.erb b/app/views/experts/show.html.erb index 86da7ba..371e5b0 100644 --- a/app/views/experts/show.html.erb +++ b/app/views/experts/show.html.erb @@ -8,7 +8,7 @@ -

+

Find an expert with the skill you need:
<%= form_for( @expert, url: search_experts_expert_path, @@ -29,6 +29,9 @@

Friends

+ <%- if current_user %> +
<%= link_to "Manage Friends", friends_expert_path(@expert.id), class: 'btn btn-sm btn-primary' %>
+ <% end %> <% @expert.friends.each do |friend| %>

diff --git a/app/views/pages/about.html.erb b/app/views/pages/about.html.erb index 6f2eb99..6fa036b 100644 --- a/app/views/pages/about.html.erb +++ b/app/views/pages/about.html.erb @@ -1,2 +1,3 @@

Experts

Welcome to the Experts Exchange

+

<%= link_to "Find experts", experts_path %> who will meet your needs!

diff --git a/spec/factories/experts.rb b/spec/factories/experts.rb index a4bc514..6e82a19 100644 --- a/spec/factories/experts.rb +++ b/spec/factories/experts.rb @@ -1,6 +1,6 @@ FactoryBot.define do factory :expert do - name {"Jane Expert"} + sequence(:name) { |n| "Jane Expert#{n}" } url {"https://en.wikipedia.org/wiki/Dog_training"} end end diff --git a/spec/factories/friendships.rb b/spec/factories/friendships.rb new file mode 100644 index 0000000..8536f37 --- /dev/null +++ b/spec/factories/friendships.rb @@ -0,0 +1,6 @@ +FactoryBot.define do + factory :friendship do + expert_1_id { 1 } + expert_2_id { 2 } + end +end diff --git a/spec/models/expert_spec.rb b/spec/models/expert_spec.rb index 42bee18..1eff6d1 100644 --- a/spec/models/expert_spec.rb +++ b/spec/models/expert_spec.rb @@ -18,5 +18,55 @@ expect(expert).not_to be_valid end + it "should handle invalid urls" do + expert = build(:expert, url: 'wombat sox') + expect(expert).not_to be_valid + end + end + + context "when finding a path to a friend" do + + let!(:expert_1) { create :expert } + let!(:expert_2) { create :expert } + let!(:expert_3) { create :expert } + let!(:expert_4) { create :expert } + + let!(:friendship_1_2) { create :friendship, expert_1_id: expert_1.id, expert_2_id: expert_2.id } + let!(:friendship_2_3) { create :friendship, expert_1_id: expert_2.id, expert_2_id: expert_3.id } + let!(:friendship_3_4) { create :friendship, expert_1_id: expert_3.id, expert_2_id: expert_4.id } + + it "should find the identity path" do + expect(expert_1.path_to_expert(expert_1)).to eq([expert_1]) + end + + it "should find a one-step path" do + expect(expert_1.path_to_expert(expert_2)).to eq([expert_1, expert_2]) + end + + it "should find a two-step path" do + expect(expert_1.path_to_expert(expert_3)).to eq([expert_1, expert_2, expert_3]) + end + + it "should find a three-step path" do + expect(expert_1.path_to_expert(expert_4)).to eq([expert_1, expert_2, expert_3, expert_4]) + end + + context "with a cycle in friendships" do + + let!(:friendship_3_1) { create :friendship, expert_1_id: expert_3.id, expert_2_id: expert_1.id } + + it "should find the shortest path without getting lost in the circle" do + expect(expert_1.path_to_expert(expert_4)).to eq([expert_1, expert_3, expert_4]) + end + end + + context "with an unreachable expert" do + + let!(:expert_5) { create :expert } + + it "should not find a path" do + expect(expert_1.path_to_expert(expert_5)).to be_falsey + end + end end end diff --git a/spec/models/friendship_spec.rb b/spec/models/friendship_spec.rb new file mode 100644 index 0000000..95567f9 --- /dev/null +++ b/spec/models/friendship_spec.rb @@ -0,0 +1,26 @@ +require 'rails_helper' + +RSpec.describe Friendship, type: :model do + + let(:expert_1) { create :expert } + let(:expert_2) { create :expert } + let(:valid_attributes) { { expert_1_id: expert_1.id, expert_2_id: expert_2.id } } + + context "when creating a friendship" do + it "is valid with expected valid attributes" do + expect(Friendship.new(valid_attributes)).to be_valid + end + + it "requires expert 1" do + expect(Friendship.new(valid_attributes.merge(expert_1_id: nil))).not_to be_valid + end + + it "requires expert 2" do + expect(Friendship.new(valid_attributes.merge(expert_2_id: nil))).not_to be_valid + end + + it "doesn't allow experts to be friends with themselves" do + expect(Friendship.new(valid_attributes.merge(expert_2_id: expert_1.id))).not_to be_valid + end + end +end