Fix GitHub Repo feature (#78)

* Create MarkdownFixer spec

* Fix lint

* Fix lint

* Update GithubRepo::find_or_create

* Fix failing specs

* Fix failing specs

* Add error handling for GithubReposController

* Improve GithubRepo::update_to_latest

app/controllers/github_repos_controller.rb

@@ -1,14 +1,23 @@
 class GithubReposController < ApplicationController
   def create
     @client = create_octokit_client
-    @repo = GithubRepo.find_or_create(github_repo_params[:url], fetched_repo_params)
-    redirect_to "/settings/integrations", notice: "GitHub repo added"
+    @repo = GithubRepo.find_or_create(fetched_repo_params)
+    if @repo.valid?
+      redirect_to "/settings/integrations", notice: "GitHub repo added"
+    else
+      redirect_to "/settings/integrations",
+        error: "There was an error adding your Github repo"
+    end
   end
 
   def update
     @repo = GithubRepo.find(params[:id])
-    @repo.update(featured: false)
-    redirect_to "/settings/integrations", notice: "GitHub repo added"
+    if @repo.update(featured: false)
+      redirect_to "/settings/integrations", notice: "GitHub repo added"
+    else
+      redirect_to "/settings/integrations",
+        error: "There was an error removing your Github repo"
+    end
   end
 
   private

app/controllers/users_controller.rb

@@ -29,14 +29,14 @@ class UsersController < ApplicationController
     @user = current_user
     @tab_list = tab_list(@user)
     @tab = params["user"]["tab"] || "profile"
-      if @user.update(user_params)
-        RssReader.new.delay.fetch_user(@user) if @user.feed_url.present?
-        notice = "Your profile was successfully updated."
-        follow_hiring_tag(@user)
-        redirect_to "/settings/#{@tab}", notice: notice
-      else
-        render :edit
-      end
+    if @user.update(user_params)
+      RssReader.new.delay.fetch_user(@user) if @user.feed_url.present?
+      notice = "Your profile was successfully updated."
+      follow_hiring_tag(@user)
+      redirect_to "/settings/#{@tab}", notice: notice
+    else
+      render :edit
+    end
   end
 
   def onboarding_update

app/models/github_repo.rb

@@ -1,31 +1,37 @@
 class GithubRepo < ApplicationRecord
   belongs_to :user
 
-  validates :name, :url, presence: true
+  validates :name, :url, :github_id_code, presence: true
   validates :url, uniqueness: true
   validates :github_id_code, uniqueness: true
 
   after_save :clear_caches
   before_destroy :clear_caches
 
-  def self.find_or_create(github_url, params = {})
-    repo = where(url: github_url).first_or_initialize
+  def self.find_or_create(params)
+    repo = where(github_id_code: params[:github_id_code]).or(where(url: params[:url])).
+      first_or_initialize
     repo.update(params)
     repo
   end
 
   def self.update_to_latest
+    # TODO: this is a very intensive process. Definitely not a good approach on the long run.
     where("updated_at < ?", 1.day.ago).find_each do |repo|
       user_token = User.find_by_id(repo.user_id).identities.where(provider: "github").last.token
       client = Octokit::Client.new(access_token: user_token)
-      fetched_repo = client.repositories.select do |fresh_repo|
-        if repo[:github_id_code]
-          fresh_repo.id == repo[:github_id_code]
-        else
-          fresh_repo.html_url == repo[:url]
-        end
-      end.first
-      repo.update(
+
+      fetched_repo = if repo[:github_id_code]
+                       client.repositories.select do |fresh_repo|
+                         fresh_repo.id == repo[:github_id_code]
+                       end.first
+                     else
+                       client.repositories.select do |fresh_repo|
+                         fresh_repo.html_url == repo[:url]
+                       end.first
+                     end
+
+      repo.update!(
         github_id_code: fetched_repo.id,
         name: fetched_repo.name,
         description: fetched_repo.description,

app/views/users/_integrations.html.erb

@@ -2,7 +2,7 @@
 <% if @client %>
   <div class="github-repos">
     <% @client.repositories.each do |repo| %>
-      <% if (user_repo = GithubRepo.find_by_name(repo.name)) && user_repo.featured %>
+      <% if (user_repo = GithubRepo.find_by(github_id_code: repo.id)) && user_repo.featured %>
         <div class="github-repo-row github-repo-row-selected">
           <div class="github-repo-row-name">
             <%= repo.name %> <%= "<span class='github-repo-fork'>fork</span>".html_safe if repo.fork %>

spec/labor/markdown_fixer_spec.rb

@@ -0,0 +1,39 @@
+require "rails_helper"
+
+RSpec.describe MarkdownFixer do
+  let(:sample_title) { Faker::Book.title }
+
+  def create_sample_markdown(title)
+    <<~HEREDOC
+      ---
+      title: #{title}
+      ---
+    HEREDOC
+  end
+
+  describe "::add_quotes_to_title" do
+    it "escapes simple title" do
+      test = described_class.fix_all(create_sample_markdown(sample_title))
+      expect(test).to eq create_sample_markdown(%("#{sample_title}"))
+    end
+
+    it "does not escape titles that came pre-wrapped in single quotes" do
+      legacy_title = "'#{sample_title}'"
+      test = described_class.fix_all(create_sample_markdown(legacy_title))
+      expect(test).to eq create_sample_markdown(legacy_title)
+    end
+
+    it "does not escape titles that came pre-wrapped in double quotes" do
+      legacy_title = "\"#{sample_title}\""
+      test = described_class.fix_all(create_sample_markdown(legacy_title))
+      expect(test).to eq create_sample_markdown(legacy_title)
+    end
+
+    it "handles complex title" do
+      legacy_title = %(Book review: "#{sample_title}", part 1 I'm #deep)
+      expected_title = "\"Book review: \\\"#{sample_title}\\\", part 1 I'm #deep\""
+      test = described_class.fix_all(create_sample_markdown(legacy_title))
+      expect(test).to eq create_sample_markdown(expected_title)
+    end
+  end
+end

spec/models/github_repo_spec.rb

@@ -1,14 +1,14 @@
 require "rails_helper"
 
 RSpec.describe GithubRepo, type: :model do
+  let(:user) { create(:user) }
+  let(:repo) { build(:github_repo, user_id: user.id) }
+
   it { is_expected.to validate_presence_of(:name) }
   it { is_expected.to validate_presence_of(:url) }
   it { is_expected.to validate_uniqueness_of(:url) }
   it { is_expected.to validate_uniqueness_of(:github_id_code) }
 
-  let(:user) { create(:user) }
-  let(:repo) { build(:github_repo, user_id: user.id) }
-
   it "saves" do
     repo.save
     expect(repo).to be_valid
@@ -16,24 +16,30 @@ RSpec.describe GithubRepo, type: :model do
 
   describe "::find_or_create" do
     it "creates a new object if one doesn't already exists" do
-      params = { name: Faker::Book.title, user_id: user.id }
-      described_class.find_or_create("#{Faker::Internet.url}1", params)
+      params = { name: Faker::Book.title, user_id: user.id, github_id_code: rand(1000),
+                 url: Faker::Internet.url }
+      described_class.find_or_create(params)
       expect(described_class.count).to eq(1)
     end
 
     it "returns existing object if it already exists" do
       repo.save
       before_update_id = repo.id
-      params = { name: Faker::Book.title }
-      updated_repo = described_class.find_or_create(repo.url, params)
+      params = { github_id_code: repo.github_id_code }
+      updated_repo = described_class.find_or_create(params)
       expect(updated_repo.id).to eq(before_update_id)
     end
   end
 
   describe "::update_to_latest" do
     let(:my_ocktokit_client) { instance_double(Octokit::Client) }
+    let(:url_of_repos_without_github_id) { Faker::Internet.url }
+    let(:repo_without_github_id) do
+      create(:github_repo, user_id: user.id, url: url_of_repos_without_github_id)
+    end
     let(:stubbed_github_repos) do
-      [OpenStruct.new(repo.attributes.merge(id: repo.github_id_code, html_url: repo.url))]
+      [OpenStruct.new(repo.attributes.merge(id: repo.github_id_code, html_url: repo.url)),
+       OpenStruct.new(repo.attributes.merge(id: rand(10000), html_url: url_of_repos_without_github_id))]
     end
 
     before do
@@ -46,15 +52,16 @@ RSpec.describe GithubRepo, type: :model do
       old_updated_at = repo.updated_at
       Timecop.freeze(Date.today + 3) do
         described_class.update_to_latest
-        expect(old_updated_at).not_to eq(GithubRepo.first.updated_at)
+        expect(old_updated_at).not_to eq(GithubRepo.find(repo.id).updated_at)
       end
     end
 
     it "uses repo's url as reference if id isn't provided" do
-      repo.update(github_id_code: nil)
+      repo_without_github_id.update_columns(github_id_code: nil)
+      old_updated_at = repo_without_github_id.updated_at
       Timecop.freeze(Date.today + 3) do
         described_class.update_to_latest
-        expect(repo.updated_at).not_to eq(GithubRepo.first.updated_at)
+        expect(old_updated_at).not_to eq(GithubRepo.find(repo_without_github_id.id).updated_at)
       end
     end
   end

spec/requests/github_repos_spec.rb

@@ -5,7 +5,7 @@ RSpec.describe "GithubRepos", type: :request do
   let(:repo) { build(:github_repo, user_id: user.id) }
   let(:my_ocktokit_client) { instance_double(Octokit::Client) }
   let(:stubbed_github_repos) do
-    [OpenStruct.new(repo.attributes.merge(id: repo.github_id_code, html_url: repo.url))]
+    [OpenStruct.new(repo.attributes.merge(id: repo.github_id_code, html_url: Faker::Internet.url))]
   end
 
   before do