From 2dd8988555dc3767758a3742b649d648098de1cc Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Thu, 10 Jan 2013 13:08:00 -0800 Subject: [PATCH 01/71] Add 'completed_at' column to 'comments' table for keeping track of action items. --- .../20130110130511_add_completed_at_to_comments.rb | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 migrations/20130110130511_add_completed_at_to_comments.rb diff --git a/migrations/20130110130511_add_completed_at_to_comments.rb b/migrations/20130110130511_add_completed_at_to_comments.rb new file mode 100644 index 00000000..3bba8d7f --- /dev/null +++ b/migrations/20130110130511_add_completed_at_to_comments.rb @@ -0,0 +1,9 @@ +require "bundler/setup" +require "pathological" +require "migrations/migration_helper.rb" + +Sequel.migration do + change do + add_column :comments, :completed_at, DateTime + end +end From 267771554af9efb0b5108a2905411de3ff50f80c Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 14 Jan 2013 17:12:17 -0800 Subject: [PATCH 02/71] Add support to the barkeep server for resolving comments. --- barkeep_server.rb | 15 +++++++++++++++ models/comment.rb | 8 ++++++++ 2 files changed, 23 insertions(+) diff --git a/barkeep_server.rb b/barkeep_server.rb index d2176e7e..36ab35fd 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -354,6 +354,20 @@ def ensure_required_params(*required_params) nil end + post "/resolve_comment" do + comment = validate_comment(params[:comment_id]) + comment.mark_resolved + comment.save + nil + end + + post "/unresolve_comment" do + comment = validate_comment(params[:comment_id]) + comment.mark_unresolved + comment.save + nil + end + post "/approve_commit" do commit = MetaRepo.instance.db_commit(params[:repo_name], params[:commit_sha]) halt 400 unless commit @@ -574,6 +588,7 @@ def create_comment(repo_name, sha, filename, line_number_string, text) :line_number => line_number, :user => current_user, :text => text, + :completed_at => nil, :has_been_emailed => current_user.demo?) # Don't email comments made by demo users. comment end diff --git a/models/comment.rb b/models/comment.rb index 5e14d6d8..f2d99f14 100644 --- a/models/comment.rb +++ b/models/comment.rb @@ -31,4 +31,12 @@ def general_comment?() commit_file_id.nil? end # True if this comment pertains to a particular file. def file_comment?() !commit_file_id.nil? end + + def mark_resolved + self.completed_at = Time.now + end + + def mark_unresolved + self.completed_at = nil + end end From 19c2df38e12da08e00927f25adf8f68c07e4b56c Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 14 Jan 2013 17:13:23 -0800 Subject: [PATCH 03/71] Add a 'Resolve' button to comments. --- public/coffee/commit.coffee | 28 ++++++++++++++++++++++++++++ public/css/styles.scss | 28 +++++++++++++++++++++++++++- views/_comment.erb | 19 +++++++++++++------ 3 files changed, 68 insertions(+), 7 deletions(-) diff --git a/public/coffee/commit.coffee b/public/coffee/commit.coffee index 60ecaa17..eda96fa8 100644 --- a/public/coffee/commit.coffee +++ b/public/coffee/commit.coffee @@ -23,6 +23,8 @@ window.Commit = $("#disapproveButton").live "click", (e) => @onDisapproveClicked e $(".delete").live "click", (e) => @onCommentDelete e $(".edit").live "click", (e) => @onCommentEdit e + $(".resolve").live "click", (e) => @onCommentResolved e + $(".unresolve").live "click", (e) => @onCommentUnresolved e $("#sideBySideButton").live "click", => @toggleSideBySide true $("#requestReviewButton").click (e) => @toggleReviewRequest() $("#hideCommentButton").live "click", (e) => @toggleComments() @@ -385,6 +387,32 @@ window.Commit = comment.find(".commentBody").html(html) comment.find(".commentCancel").click() + onCommentResolved: (e) -> + commentId = $(e.target).parents(".comment").attr("commentId") + $.ajax + type: "post", + url: "/resolve_comment", + data: { comment_id: commentId }, + success: => + comment = $(".comment[commentId='#{commentId}']") + button = comment.find(".resolve") + $(button).html("Unresolve") + $(button).removeClass("resolve") + $(button).addClass("unresolve") + + onCommentUnresolved: (e) -> + commentId = $(e.target).parents(".comment").attr("commentId") + $.ajax + type: "post", + url: "/unresolve_comment", + data: { comment_id: commentId }, + success: => + comment = $(".comment[commentId='#{commentId}']") + button = comment.find(".unresolve") + $(button).html("Resolve") + $(button).removeClass("unresolve") + $(button).addClass("resolve") + # TODO(caleb): Add a Snippet for comment forms instead of contacting the server (there's no server logic # needed here). createCommentForm: (codeLine, repoName, sha, filename, lineNumber) -> diff --git a/public/css/styles.scss b/public/css/styles.scss index 6d261b78..0a5fc8d5 100644 --- a/public/css/styles.scss +++ b/public/css/styles.scss @@ -144,7 +144,7 @@ button.fancy { } /* Our standard buttons are flat grey buttons. */ -button.standard, input[type=button], input[type=submit] { +button.standard, .unresolve, input[type=button], input[type=submit] { border-radius: 3px; padding: 7px 12px; text-shadow: 0px -1px 1px #EEEEEE; @@ -166,6 +166,32 @@ button.standard, input[type=button], input[type=submit] { .webkit button.standard { font-weight: bold; } +/* The "Resolve" button needs to stand out a bit from the other buttons because it indicates + * the state of the comment and users will want to find all their unresolved comments. + * This style is a hybrid between the standard buttons (Reply, Edit, Delete) and the "Approve Commit" + * button. + */ +button.resolve { + $resolveColor: #42C050; + border-radius: 3px; + padding: 7px 12px; + border: 1px solid darken($resolveColor, 20%); + border-bottom: 1px solid darken($resolveColor, 25%); + text-shadow: 0px -1px 1px darken($resolveColor, 30%); + font-weight: bold; + cursor: pointer; + color: white; + + @include linear-gradient(lighten($resolveColor, 3%) 0%, darken($resolveColor, 3%) 95%); + box-shadow: 0px 1px 0px rgba(255, 255, 255, 0.7); + &:hover { + @include linear-gradient(lighten($resolveColor, 4%) 0%, darken($resolveColor, 1%) 95%); + } + &:active { + @include linear-gradient(darken($resolveColor, 3%), lighten($resolveColor, 3%) 95%); + } +} + /* jQuery autocomplete styles. */ .ui-widget { /* This needs some width set, but it will fit itself to the size of the autocompleting input box. */ diff --git a/views/_comment.erb b/views/_comment.erb index 05bd8184..6f1468d1 100644 --- a/views/_comment.erb +++ b/views/_comment.erb @@ -9,12 +9,19 @@ commented <%= comment.created_at.to_pretty %>
- <% if current_user && comment.line_number %> - - <% end %> - <% if current_user && current_user.id == comment.user_id %> - - + <% if current_user %> + <% if comment.completed_at %> + + <% else %> + + <% end %> + <% if comment.line_number %> + + <% end %> + <% if current_user.id == comment.user_id %> + + + <% end %> <% end %>
From a427b28517dc04178321175dae098b03d17b71bc Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 14 Jan 2013 17:18:10 -0800 Subject: [PATCH 04/71] Style the 'Resolve' button more like the 'Approve Commit' button. --- public/css/styles.scss | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/public/css/styles.scss b/public/css/styles.scss index 0a5fc8d5..339cd377 100644 --- a/public/css/styles.scss +++ b/public/css/styles.scss @@ -182,14 +182,16 @@ button.resolve { cursor: pointer; color: white; - @include linear-gradient(lighten($resolveColor, 3%) 0%, darken($resolveColor, 3%) 95%); - box-shadow: 0px 1px 0px rgba(255, 255, 255, 0.7); + @include linear-gradient(lighten($resolveColor, 3%) 0%, darken($resolveColor, 15%) 95%, + $fallback: darken($resolveColor, 18%)); + &:hover { - @include linear-gradient(lighten($resolveColor, 4%) 0%, darken($resolveColor, 1%) 95%); - } - &:active { - @include linear-gradient(darken($resolveColor, 3%), lighten($resolveColor, 3%) 95%); + border: 1px solid darken(#890128, 25%); + @include linear-gradient(lighten($resolveColor, 10%) 0%, darken($resolveColor, 15%) 95%, + $fallback: darken($resolveColor, 18%)); } + + &:active { background-image: none; } } /* jQuery autocomplete styles. */ From f90511165051bbc0456a6408a1a3cfd6ab3a0f11 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 14 Jan 2013 20:07:01 -0800 Subject: [PATCH 05/71] Do not display the 'Resolve' button for comments and replies by the commit author. --- views/_comment.erb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/views/_comment.erb b/views/_comment.erb index 6f1468d1..b2fc9ff9 100644 --- a/views/_comment.erb +++ b/views/_comment.erb @@ -10,10 +10,12 @@ <%= comment.created_at.to_pretty %>
<% if current_user %> - <% if comment.completed_at %> - - <% else %> - + <% if current_user.email != comment.commit.grit_commit.author.email %> + <% if comment.completed_at %> + + <% else %> + + <% end %> <% end %> <% if comment.line_number %> From 77265b16085e8b4f0a73fc0439d21164e2bcab1b Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 14 Jan 2013 20:21:12 -0800 Subject: [PATCH 06/71] Allow any user to resolve and unresolve comments. --- barkeep_server.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index 36ab35fd..6584210d 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -355,14 +355,14 @@ def ensure_required_params(*required_params) end post "/resolve_comment" do - comment = validate_comment(params[:comment_id]) + comment = validate_comment(params[:comment_id], true) comment.mark_resolved comment.save nil end post "/unresolve_comment" do - comment = validate_comment(params[:comment_id]) + comment = validate_comment(params[:comment_id], true) comment.mark_unresolved comment.save nil @@ -593,10 +593,12 @@ def create_comment(repo_name, sha, filename, line_number_string, text) comment end - def validate_comment(comment_id) + def validate_comment(comment_id, allow_any_user = false) comment = Comment[comment_id] halt 404, "This comment no longer exists." unless comment - halt 403, "Comment not originated from this user." unless comment.user.id == current_user.id + if !allow_any_user && comment.user.id != current_user.id + halt 403, "Comment not originated from this user." + end comment end end From f873655ce6ea3c8e64b939b842fe62c728945a62 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Tue, 15 Jan 2013 13:55:34 -0800 Subject: [PATCH 07/71] Add 'Reviews' button to main layout. --- public/css/styles.scss | 4 ++-- views/layout.erb | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/public/css/styles.scss b/public/css/styles.scss index 339cd377..af6c9a6a 100644 --- a/public/css/styles.scss +++ b/public/css/styles.scss @@ -282,7 +282,7 @@ header { list-style-type: none; display: inline-block; padding-bottom: 6px; - margin-right: 20px; + margin-right: 12px; /* This extra div is needed to add a white shadow around the tag. */ div { @@ -294,7 +294,7 @@ header { border-top: 0; display: block; - width: 120px; + width: 90px; height: 27px; background-color: $sunkenColor; diff --git a/views/layout.erb b/views/layout.erb index 13f43694..712e9ab6 100644 --- a/views/layout.erb +++ b/views/layout.erb @@ -52,6 +52,9 @@
  • ">
    • +
  • "> + +
  • ">
    • From 48b0a08bb3601790024f51c9cdc6213bf0a9ab61 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Tue, 15 Jan 2013 14:02:01 -0800 Subject: [PATCH 08/71] Do not show 'Reviews' button unless user is signed in. --- views/layout.erb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/views/layout.erb b/views/layout.erb index 712e9ab6..9d5fea22 100644 --- a/views/layout.erb +++ b/views/layout.erb @@ -52,9 +52,11 @@
  • ">
    • -
  • "> - -
    • + <% unless current_user.nil? || current_user.demo? %> +
  • "> + +
    • + <% end %>
  • ">
    • From d57af88cee3b3885b8c929ead4c6af2ff6948520 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Wed, 16 Jan 2013 13:49:10 -0800 Subject: [PATCH 09/71] Populate the 'authors' table with unique commit authors. --- .../20130116081012_populate_authors_table.rb | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 migrations/20130116081012_populate_authors_table.rb diff --git a/migrations/20130116081012_populate_authors_table.rb b/migrations/20130116081012_populate_authors_table.rb new file mode 100644 index 00000000..b8584a83 --- /dev/null +++ b/migrations/20130116081012_populate_authors_table.rb @@ -0,0 +1,53 @@ +require "bundler/setup" +require "pathological" +require "migrations/migration_helper.rb" +require "grit" + +# This migration populates the "authors" table by fetching all the commits from all the repos +# and storing the unique commit authors in the "authors" table. If it finds an identical email +# address in the "users" table then it also adds the "user_id" to the authors table entry. + +# This is the number of commits that we fetch at a time. +PAGE_SIZE = 100 + +Sequel.migration do + up do + repos = DB[:git_repos].all + + # Find all the unique authors + authors = Hash.new { |hash, key| hash[key] = {} } + repos.each do |repo| + grit_repo = Grit::Repo.new(repo[:path]) + total = 0 + num = 0 + begin + commits = grit_repo.commits("master", PAGE_SIZE, total) + commits.each { |commit| authors[commit.author.email][:name] = commit.author.name } + num = commits.length + total += num + end while num == PAGE_SIZE + puts "Processed #{total} commits from repo #{repo[:path]}." + end + + # Find matching users (by email) in the "users" table. + authors.keys.each do |email| + user = DB[:users].first(:email => email) + authors[email][:user_id] = user[:id] if user + end + puts "Found #{authors.length} unique authors." + + # Fill in the "authors" table. + num_inserts = 0 + authors.each do |key, value| + row = DB[:authors].first(:email => key) + next if row + DB[:authors].insert(:email => key, :name => value[:name], :user_id => value[:user_id]) + num_inserts += 1 + end + puts "Inserted #{num_inserts} new authors." + end + + # We don't need to remove the author entries. + down do + end +end From 3a01ed36d71b06c156c9fec9e838cfff8b9bb580 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Wed, 16 Jan 2013 15:20:06 -0800 Subject: [PATCH 10/71] Add author_id field to 'commits' table. --- migrations/20130116151408_add_author_id_to_commits.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 migrations/20130116151408_add_author_id_to_commits.rb diff --git a/migrations/20130116151408_add_author_id_to_commits.rb b/migrations/20130116151408_add_author_id_to_commits.rb new file mode 100644 index 00000000..263f1730 --- /dev/null +++ b/migrations/20130116151408_add_author_id_to_commits.rb @@ -0,0 +1,11 @@ +require "bundler/setup" +require "pathological" +require "migrations/migration_helper.rb" + +Sequel.migration do + change do + alter_table :commits do + add_foreign_key :author_id, :authors, :key => :id + end + end +end From 999e0b554af3abc9bbaa414de98fad5dcb383bc6 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Wed, 16 Jan 2013 16:25:37 -0800 Subject: [PATCH 11/71] Populate the author_id field in the commits table. --- ...20130116152425_populate_author_id_field.rb | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 migrations/20130116152425_populate_author_id_field.rb diff --git a/migrations/20130116152425_populate_author_id_field.rb b/migrations/20130116152425_populate_author_id_field.rb new file mode 100644 index 00000000..7de5d713 --- /dev/null +++ b/migrations/20130116152425_populate_author_id_field.rb @@ -0,0 +1,62 @@ +require "bundler/setup" +require "pathological" +require "migrations/migration_helper.rb" +require "grit" + +# This migration populates the "author_id" field in the "commits" table. + +# This is the number of commits that we fetch at a time. +PAGE_SIZE = 100 + +Sequel.migration do + up do + repos = DB[:git_repos].all + + # Fetch all the authors (expected to be small, say less than 1000) so that we don't have to do + # an extra SQL query for every commit to check if the author email exists. Then create a mapping + # from email -> author.id + rows = DB[:authors].all + puts "Read #{rows.length} rows from the 'authors' table." + authors = {} + rows.each do |row| + authors[row[:email]] = row[:id] + end + + # Create a mapping from commit sha -> author email + shas = Hash.new { |hash, key| hash[key] = {} } + repos.each do |repo| + grit_repo = Grit::Repo.new(repo[:path]) + total = 0 + num = 0 + begin + commits = grit_repo.commits("master", PAGE_SIZE, total) + commits.each do |commit| + email = commit.author.email + author_id = authors[email] + if author_id.nil? + # This shouldn't happen. We should already have the author in our db. But if not, + # add the author to the db. + user = DB[:users].first(:email => email) + user_id = user[:id] if user + DB[:authors].insert(:email => email, :name => commit.author.name, :user_id => user_id) + + # Get the author_id and add it to the hash. + author = DB[:authors].first(:email => email) + author_id = author[:id] + authors[email] = author_id + end + + # Update the author_id field. + DB[:commits].filter(:sha => commit.sha).update(:author_id => author_id) + end + num = commits.length + total += num + end while num == PAGE_SIZE + puts "Processed #{total} commits from repo #{repo[:path]}." + end + end + + # We don't need to undo this. + down do + end +end From 2203c968a988a2c81435d7be38e4e9bdfffce9b4 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Wed, 16 Jan 2013 17:55:30 -0800 Subject: [PATCH 12/71] Insert new commit authors into 'authors' table when new commits are ingested. This also fills in the "author_id" field for new commits. --- lib/models.rb | 1 + models/author.rb | 4 ++++ models/user.rb | 1 + resque_jobs/db_commit_ingest.rb | 16 ++++++++++++++++ 4 files changed, 22 insertions(+) create mode 100644 models/author.rb diff --git a/lib/models.rb b/lib/models.rb index 09adbc3c..52b8c35e 100644 --- a/lib/models.rb +++ b/lib/models.rb @@ -11,6 +11,7 @@ # Auto-populate "created_at" and "updated_at" fields. Sequel::Model.plugin :timestamps +require "models/author" require "models/git_repo" require "models/git_branch" require "models/user" diff --git a/models/author.rb b/models/author.rb new file mode 100644 index 00000000..883bf424 --- /dev/null +++ b/models/author.rb @@ -0,0 +1,4 @@ +class Author < Sequel::Model + # This is really one_to_one, but Sequel requires the table containing the foreign key to be many_to_one. + many_to_one :user +end diff --git a/models/user.rb b/models/user.rb index 37ae46e6..6ade5607 100644 --- a/models/user.rb +++ b/models/user.rb @@ -12,6 +12,7 @@ class User < Sequel::Model one_to_many :saved_searches, :order => [:user_order.desc] one_to_many :comments + one_to_one :author ONE_YEAR = 365 diff --git a/resque_jobs/db_commit_ingest.rb b/resque_jobs/db_commit_ingest.rb index 5186645b..afcdf87c 100644 --- a/resque_jobs/db_commit_ingest.rb +++ b/resque_jobs/db_commit_ingest.rb @@ -44,6 +44,7 @@ def self.perform(repo_name, remote_name) page_of_rows_to_insert = commits.map do |commit| next if existing_shas.include?(commit.sha) + author_id = insert_author_if_new(commit) { :git_repo_id => db_repo.id, @@ -52,6 +53,7 @@ def self.perform(repo_name, remote_name) # NOTE(caleb): For some reason, the commit object you get from a remote returns nil for #date (but # it does have #authored_date and #committed_date. Bug? :date => commit.authored_date, + :author_id => author_id, } end page_of_rows_to_insert.compact! @@ -78,4 +80,18 @@ def self.perform(repo_name, remote_name) Resque.enqueue(GenerateTaggedDiffs, repo_name, row[:sha]) end end + + # Given a new commit, insert the author into the authors table if the author does not exist. + # In any case, returns the author id. + def self.insert_author_if_new(commit) + email = commit.author.email + author = Author.first(:email => email) + return author.id if author + + user = User.first(:email => email) + user_id = user.id if user + Author.insert(:email => email, :name => commit.author.name, :user_id => user_id) + author = Author.first(:email => email) + author.id + end end From 3d56362c640062fc21ff668e6b13b3a34dbf7bb9 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Thu, 17 Jan 2013 11:13:35 -0800 Subject: [PATCH 13/71] Link authors to users when they sign in. --- barkeep_server.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/barkeep_server.rb b/barkeep_server.rb index d2176e7e..a001fb61 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -247,6 +247,7 @@ def ensure_required_params(*required_params) permission = User.find(:permission => "admin").nil? ? "admin" : "normal" User.new(:email => email, :name => email, :permission => permission).save end + link_author_to_user(email) redirect session[:login_started_url] || "/" end end @@ -559,6 +560,19 @@ def get_openid_login_redirect(openid_provider_url) end end + # If the email matches an entry in both the authors table and the users table, then create + # a link between them by updating the "user_id" field in the authors table. + def link_author_to_user(email) + author = Author.first(:email => email) + if author && author.user_id.nil? + user = User.first(:email => email) + if user + author.user_id = user.id + author.save + end + end + end + def create_comment(repo_name, sha, filename, line_number_string, text) commit = MetaRepo.instance.db_commit(repo_name, sha) raise "No such commit." unless commit From 31198ce1b5eb8cf3af067f9dcabcbbbe7d8a12aa Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Thu, 17 Jan 2013 13:07:08 -0800 Subject: [PATCH 14/71] Modified the migration that populates the author_id field to iterate over the db commits (instead of Grit). --- ...20130116152425_populate_author_id_field.rb | 67 +++++++++---------- 1 file changed, 30 insertions(+), 37 deletions(-) diff --git a/migrations/20130116152425_populate_author_id_field.rb b/migrations/20130116152425_populate_author_id_field.rb index 7de5d713..35800f4c 100644 --- a/migrations/20130116152425_populate_author_id_field.rb +++ b/migrations/20130116152425_populate_author_id_field.rb @@ -5,55 +5,48 @@ # This migration populates the "author_id" field in the "commits" table. -# This is the number of commits that we fetch at a time. -PAGE_SIZE = 100 - Sequel.migration do up do - repos = DB[:git_repos].all - # Fetch all the authors (expected to be small, say less than 1000) so that we don't have to do # an extra SQL query for every commit to check if the author email exists. Then create a mapping # from email -> author.id rows = DB[:authors].all - puts "Read #{rows.length} rows from the 'authors' table." authors = {} rows.each do |row| authors[row[:email]] = row[:id] end - # Create a mapping from commit sha -> author email - shas = Hash.new { |hash, key| hash[key] = {} } - repos.each do |repo| - grit_repo = Grit::Repo.new(repo[:path]) - total = 0 - num = 0 - begin - commits = grit_repo.commits("master", PAGE_SIZE, total) - commits.each do |commit| - email = commit.author.email - author_id = authors[email] - if author_id.nil? - # This shouldn't happen. We should already have the author in our db. But if not, - # add the author to the db. - user = DB[:users].first(:email => email) - user_id = user[:id] if user - DB[:authors].insert(:email => email, :name => commit.author.name, :user_id => user_id) - - # Get the author_id and add it to the hash. - author = DB[:authors].first(:email => email) - author_id = author[:id] - authors[email] = author_id - end - - # Update the author_id field. - DB[:commits].filter(:sha => commit.sha).update(:author_id => author_id) - end - num = commits.length - total += num - end while num == PAGE_SIZE - puts "Processed #{total} commits from repo #{repo[:path]}." + # Fetch all the git repos (also small, less than 100) and create a mapping from + # git_repo_id -> Grit::Repo + repos = {} + DB[:git_repos].each { |row| repos[row[:id]] = Grit::Repo.new(row[:path]) } + + total_updates = 0 + new_authors = 0 + commits = DB[:commits].filter(:author_id => nil).all + commits.each do |row| + commit = repos[row[:git_repo_id]].commit(row[:sha]) + next unless commit + email = commit.author.email + author_id = authors[email] + # If the author is not in our db, then add it. + if author_id.nil? + # Check if the same email exists in the users table. + user = DB[:users].first(:email => email) + user_id = user[:id] if user + DB[:authors].insert(:email => email, :name => commit.author.name, :user_id => user_id) + + # Get the author_id and add it to the hash. + author = DB[:authors].first(:email => email) + author_id = author[:id] + authors[email] = author_id + new_authors += 1 + end + total_updates += 1 + DB[:commits].filter(:id => row[:id]).update(:author_id => authors[email]) end + puts "New authors: #{new_authors}" + puts "Updated commits: #{total_updates}" end # We don't need to undo this. From f850dd5e1ea22348444b5a82c3fdfe73ac829704 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Thu, 17 Jan 2013 17:31:48 -0800 Subject: [PATCH 15/71] Add Sequel associations between Author and Commit. --- models/author.rb | 1 + models/commit.rb | 3 +++ 2 files changed, 4 insertions(+) diff --git a/models/author.rb b/models/author.rb index 883bf424..2af9e257 100644 --- a/models/author.rb +++ b/models/author.rb @@ -1,4 +1,5 @@ class Author < Sequel::Model # This is really one_to_one, but Sequel requires the table containing the foreign key to be many_to_one. many_to_one :user + one_to_one :commit end diff --git a/models/commit.rb b/models/commit.rb index a50da71e..de9dbc32 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -12,6 +12,9 @@ class Commit < Sequel::Model one_to_many :comments many_to_one :approved_by_user, :class => User + # This is really one_to_one, but Sequel requires the table containing the foreign key to be many_to_one. + many_to_one :author + add_association_dependencies :comments => :destroy, :commit_files => :destroy add_filter(:message) { |message| StringFilter.escape_html(message) } From 6c087c06a77252c804d58bea66309655e7b00e45 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Thu, 17 Jan 2013 17:47:08 -0800 Subject: [PATCH 16/71] Fix bug with not showing the Resolve button for the committer. --- views/_comment.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/views/_comment.erb b/views/_comment.erb index b2fc9ff9..0380e4fd 100644 --- a/views/_comment.erb +++ b/views/_comment.erb @@ -10,7 +10,7 @@ <%= comment.created_at.to_pretty %>
    <% if current_user %> - <% if current_user.email != comment.commit.grit_commit.author.email %> + <% if comment.user_id != comment.commit.author.user_id %> <% if comment.completed_at %> <% else %> From 6a4358fdf8694eb15a6ac39cd3672099ba971f16 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Thu, 17 Jan 2013 17:59:35 -0800 Subject: [PATCH 17/71] Implement the initial (simple) Reviews view. This just displays a list of commits that have unresolved comments. --- barkeep_server.rb | 7 +++++++ models/commit.rb | 18 ++++++++++++++++++ public/css/saved_search.scss | 4 ++-- views/_unresolved_commit.erb | 35 +++++++++++++++++++++++++++++++++++ views/reviews.erb | 8 ++++++++ 5 files changed, 70 insertions(+), 2 deletions(-) create mode 100644 views/_unresolved_commit.erb create mode 100644 views/reviews.erb diff --git a/barkeep_server.rb b/barkeep_server.rb index 21c80c2e..6d48a8c3 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -488,6 +488,13 @@ def ensure_required_params(*required_params) redirect "/stats" end + get "/reviews" do + commits_with_unresolved_comments = Commit.commits_with_unresolved_comments(current_user.email) + erb :reviews, :locals => { + :commits_with_unresolved_comments => commits_with_unresolved_comments, + } + end + get "/profile/:id" do user = User[params[:id]] halt 404 unless user diff --git a/models/commit.rb b/models/commit.rb index de9dbc32..fe9c01eb 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -70,4 +70,22 @@ def self.prefix_match(git_repo, partial_sha, zero_commits_ok = false) commits[0] end end + + # Fetches the commits with unresolved comments for the given email. The email is used to find + # the commits by that user and to exclude comments made by that user. + def self.commits_with_unresolved_comments(email) + commits = Commit. + join(:comments, :commit_id => :id). + join(:authors, :id => :commits__author_id). + join(:users, :id => :comments__user_id). + filter(:authors__email => email, :comments__completed_at => nil). + exclude(:users__email => email). + group_by(:commits__id).all + commits.map! do |commit| + grit_commit = MetaRepo.instance.grit_commit(commit.git_repo.name, commit.sha) + next unless grit_commit + grit_commit + end + commits.reject(&:nil?) + end end diff --git a/public/css/saved_search.scss b/public/css/saved_search.scss index 43ab6c2c..b322a9f5 100644 --- a/public/css/saved_search.scss +++ b/public/css/saved_search.scss @@ -12,7 +12,7 @@ .handle { cursor: move; } -.savedSearch, .statistic { +.savedSearch, .statistic, .review { position: relative; /* We want the background of a saved search to be opaque while we're dragging it around. */ background-image: url(/assets/images/background.png); @@ -36,7 +36,7 @@ } } -.savedSearch, .statistic.chattyCommits { +.savedSearch, .statistic.chattyCommits, .review { width: 100%; .header { diff --git a/views/_unresolved_commit.erb b/views/_unresolved_commit.erb new file mode 100644 index 00000000..351fb3d6 --- /dev/null +++ b/views/_unresolved_commit.erb @@ -0,0 +1,35 @@ +<%# + A single table row for a commit with unresolved comments. + View arguments: + - commit: a grit commit. + %> +<% db_commit = MetaRepo.instance.db_commit(commit.repo_name, commit.sha) %> + + " + title="Approved" rel="tipsy"> + +
    + +
    + + + <%= commit.author.to_s[0..25] %> + + + <%= commit.id_abbrev %> + + + <%= CGI::escapeHTML(commit.short_message) %> + + <%= commit.date.to_pretty %> + <%= commit.repo_name %> + + <% comment_count = db_commit.comment_count %> + <% if comment_count > 0 %> + "> + <%= comment_count %>" /> + + <% end %> + + diff --git a/views/reviews.erb b/views/reviews.erb new file mode 100644 index 00000000..bcf0a378 --- /dev/null +++ b/views/reviews.erb @@ -0,0 +1,8 @@ +
    +
    Commits with unresolved comments
    + + <% commits_with_unresolved_comments.each do |commit| %> + <%= erb :_unresolved_commit, :layout => false, :locals => { :commit => commit } %> + <% end %> +
    +
    From 6322dfc2fb866902a9cb75dccddf351c3e1867be Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Thu, 17 Jan 2013 21:32:04 -0800 Subject: [PATCH 18/71] Create the 'review_requests' table to keep track of code review requests. --- ...0130117210154_create_review_requests_table.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 migrations/20130117210154_create_review_requests_table.rb diff --git a/migrations/20130117210154_create_review_requests_table.rb b/migrations/20130117210154_create_review_requests_table.rb new file mode 100644 index 00000000..3915da61 --- /dev/null +++ b/migrations/20130117210154_create_review_requests_table.rb @@ -0,0 +1,16 @@ +require "bundler/setup" +require "pathological" +require "migrations/migration_helper.rb" + +Sequel.migration do + change do + create_table(:review_requests) do + primary_key :id + foreign_key :requester_user_id, :users, :key => :id + foreign_key :reviewer_user_id, :users, :key => :id + foreign_key :commit_id, :commits, :key => :id + DateTime :completed_at + DateTime :requested_at + end + end +end From b9d6c18d25d01de46f6758ce79b5e844233ddffc Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Fri, 18 Jan 2013 13:53:06 -0800 Subject: [PATCH 19/71] Track code review requests in the 'review_requests' table. --- barkeep_server.rb | 21 +++++++++++++++++++++ lib/models.rb | 1 + models/commit.rb | 1 + models/review_request.rb | 5 +++++ 4 files changed, 28 insertions(+) create mode 100644 models/review_request.rb diff --git a/barkeep_server.rb b/barkeep_server.rb index 6d48a8c3..b981b260 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -515,6 +515,7 @@ def ensure_required_params(*required_params) commit = Commit.first(:sha => params[:sha]) halt 404 unless commit emails = params[:emails].split(",").map(&:strip).reject(&:empty?) + insert_review_requests(commit.id, current_user.id, emails) Resque.enqueue(DeliverReviewRequestEmails, commit.git_repo.name, commit.sha, current_user.email, emails) nil end @@ -622,6 +623,26 @@ def validate_comment(comment_id, allow_any_user = false) end comment end + + # Tracks the review requests by inserting rows in the "review_requests" table, one for each email. + def insert_review_requests(commit_id, requester_id, emails) + emails.each do |email| + user = User.first(:email => email) + if user.nil? + # Try harder to find a match. Check if the username contains a "+". + username, domain = email.split("@", 2) + simple_username, _ = username.split("+", 2) # ignore stuff after the "+" + next if simple_username == username + simple_email = "#{simple_username}@#{domain}" + user = User.first(:email => simple_email) + next if user.nil? # Skip this review request if we can't find the user + end + ReviewRequest.insert(:requester_user_id => requester_id, + :reviewer_user_id => user.id, + :requested_at => Time.now.utc, + :commit_id => commit_id) + end + end end # These are extra routes. Require them after the main routes and before filters have been defined, so diff --git a/lib/models.rb b/lib/models.rb index 52b8c35e..b47e7017 100644 --- a/lib/models.rb +++ b/lib/models.rb @@ -20,3 +20,4 @@ require "models/commit_file" require "models/comment" require "models/completed_email" +require "models/review_request" diff --git a/models/commit.rb b/models/commit.rb index fe9c01eb..93f724d4 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -11,6 +11,7 @@ class Commit < Sequel::Model one_to_many :commit_files one_to_many :comments many_to_one :approved_by_user, :class => User + one_to_many :review_request # This is really one_to_one, but Sequel requires the table containing the foreign key to be many_to_one. many_to_one :author diff --git a/models/review_request.rb b/models/review_request.rb new file mode 100644 index 00000000..3e54b88d --- /dev/null +++ b/models/review_request.rb @@ -0,0 +1,5 @@ +class ReviewRequest < Sequel::Model + many_to_one :commit + many_to_one :requester_user, :class => User + many_to_one :reviewer_user, :class => User +end From 7743507324d8868e71dfdede27ab6847124f0bce Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Fri, 18 Jan 2013 14:52:33 -0800 Subject: [PATCH 20/71] Display the list of code review requests. --- barkeep_server.rb | 2 ++ models/review_request.rb | 12 ++++++++++++ views/reviews.erb | 9 +++++++++ 3 files changed, 23 insertions(+) diff --git a/barkeep_server.rb b/barkeep_server.rb index b981b260..ecb4e26e 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -490,8 +490,10 @@ def ensure_required_params(*required_params) get "/reviews" do commits_with_unresolved_comments = Commit.commits_with_unresolved_comments(current_user.email) + commits_with_uncompleted_reviews = ReviewRequest.commits_with_uncompleted_reviews(current_user.id) erb :reviews, :locals => { :commits_with_unresolved_comments => commits_with_unresolved_comments, + :commits_with_uncompleted_reviews => commits_with_uncompleted_reviews, } end diff --git a/models/review_request.rb b/models/review_request.rb index 3e54b88d..ab4b1920 100644 --- a/models/review_request.rb +++ b/models/review_request.rb @@ -2,4 +2,16 @@ class ReviewRequest < Sequel::Model many_to_one :commit many_to_one :requester_user, :class => User many_to_one :reviewer_user, :class => User + + def self.commits_with_uncompleted_reviews(user_id) + uncompleted = ReviewRequest.filter(:reviewer_user_id => user_id, :completed_at => nil). + group_by(:commit_id).all + commits = uncompleted.map do |review| + grit_commit = MetaRepo.instance.grit_commit(review.commit.git_repo.name, review.commit.sha) + next unless grit_commit + grit_commit + end + commits.reject!(&:nil?) + commits + end end diff --git a/views/reviews.erb b/views/reviews.erb index bcf0a378..80a27908 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -1,3 +1,12 @@ +
    +
    Code review requests
    + + <% commits_with_uncompleted_reviews.each do |commit| %> + <%= erb :_unresolved_commit, :layout => false, :locals => { :commit => commit } %> + <% end %> +
    +
    +
    Commits with unresolved comments
    From 3073bdfa408b5e5f0fb36932a91ca034bf41f29f Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Fri, 18 Jan 2013 16:45:59 -0800 Subject: [PATCH 21/71] Avoid creating multiple review_request entries for the same review request. --- barkeep_server.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index ecb4e26e..ab023d0f 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -639,10 +639,20 @@ def insert_review_requests(commit_id, requester_id, emails) user = User.first(:email => simple_email) next if user.nil? # Skip this review request if we can't find the user end - ReviewRequest.insert(:requester_user_id => requester_id, + + # If the review request does not exist, then insert a new request. Otherwise (the review + # request already exists) mark it as not completed. + review_request = ReviewRequest.first(:requester_user_id => requester_id, :reviewer_user_id => user.id, - :requested_at => Time.now.utc, :commit_id => commit_id) + if review_request.nil? + ReviewRequest.insert(:requester_user_id => requester_id, + :reviewer_user_id => user.id, + :requested_at => Time.now.utc, + :commit_id => commit_id) + elsif !review_request.completed_at.nil? + ReviewRequest.filter(:id => review_request.id).update(:completed_at => nil); + end end end end From 14adc7b3690e703c4159c12324c4f4c066d0f4ce Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Fri, 18 Jan 2013 16:51:36 -0800 Subject: [PATCH 22/71] Mark a review request as complete when a commit is approved or disapproved. --- barkeep_server.rb | 2 ++ models/review_request.rb | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/barkeep_server.rb b/barkeep_server.rb index ab023d0f..965a897b 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -373,6 +373,7 @@ def ensure_required_params(*required_params) commit = MetaRepo.instance.db_commit(params[:repo_name], params[:commit_sha]) halt 400 unless commit commit.approve(current_user) + ReviewRequest.complete_requests(commit.id) erb :_approved_banner, :layout => false, :locals => { :commit => commit } end @@ -380,6 +381,7 @@ def ensure_required_params(*required_params) commit = MetaRepo.instance.db_commit(params[:repo_name], params[:commit_sha]) halt 400 unless commit commit.disapprove + ReviewRequest.complete_requests(commit.id) nil end diff --git a/models/review_request.rb b/models/review_request.rb index ab4b1920..fc9b2940 100644 --- a/models/review_request.rb +++ b/models/review_request.rb @@ -14,4 +14,8 @@ def self.commits_with_uncompleted_reviews(user_id) commits.reject!(&:nil?) commits end + + def self.complete_requests(commit_id) + ReviewRequest.filter(:commit_id => commit_id).update(:completed_at => Time.now.utc) + end end From c912309ccd857a97abf1dd366c3f3ad2d31a25dc Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sat, 19 Jan 2013 10:26:00 -0800 Subject: [PATCH 23/71] Display code review requests (and recently resolved requests). --- barkeep_server.rb | 10 ++++++++++ models/review_request.rb | 14 +++++++++++++ public/coffee/reviews.coffee | 23 ++++++++++++++++++++++ public/css/saved_search.scss | 9 +++++++++ views/_review_request.erb | 38 ++++++++++++++++++++++++++++++++++++ views/reviews.erb | 19 ++++++++++++++---- 6 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 public/coffee/reviews.coffee create mode 100644 views/_review_request.erb diff --git a/barkeep_server.rb b/barkeep_server.rb index 965a897b..fcac378b 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -91,6 +91,7 @@ class BarkeepServer < Sinatra::Base "/vendor/flot/jquery.flot.min.js", "/vendor/flot/jquery.flot.pie.min.js" ] + pinion.create_bundle :reviews_app_js, :concatenate_and_uglify_js, ["/coffee/reviews.js"] pinion.create_bundle :user_settings_app_js, :concatenate_and_uglify_js, ["/coffee/user_settings.js"] end @@ -385,6 +386,13 @@ def ensure_required_params(*required_params) nil end + post "/complete_review_request" do + commit = MetaRepo.instance.db_commit(params[:repo_name], params[:commit_sha]) + halt 400 unless commit + ReviewRequest.complete_requests(commit.id) + nil + end + # Saves changes to the user-level search options. post "/user_search_options" do saved_search_time_period = params[:saved_search_time_period].to_i @@ -493,9 +501,11 @@ def ensure_required_params(*required_params) get "/reviews" do commits_with_unresolved_comments = Commit.commits_with_unresolved_comments(current_user.email) commits_with_uncompleted_reviews = ReviewRequest.commits_with_uncompleted_reviews(current_user.id) + recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) erb :reviews, :locals => { :commits_with_unresolved_comments => commits_with_unresolved_comments, :commits_with_uncompleted_reviews => commits_with_uncompleted_reviews, + :recently_reviewed_commits => recently_reviewed_commits, } end diff --git a/models/review_request.rb b/models/review_request.rb index fc9b2940..c1965bf1 100644 --- a/models/review_request.rb +++ b/models/review_request.rb @@ -15,6 +15,20 @@ def self.commits_with_uncompleted_reviews(user_id) commits end + def self.recently_reviewed_commits(user_id) + recently_reviewed = ReviewRequest.filter(:reviewer_user_id => user_id). + exclude(:completed_at => nil). + group_by(:commit_id). + reverse_order(:completed_at).limit(5).all + commits = recently_reviewed.map do |review| + grit_commit = MetaRepo.instance.grit_commit(review.commit.git_repo.name, review.commit.sha) + next unless grit_commit + grit_commit + end + commits.reject!(&:nil?) + commits + end + def self.complete_requests(commit_id) ReviewRequest.filter(:commit_id => commit_id).update(:completed_at => Time.now.utc) end diff --git a/public/coffee/reviews.coffee b/public/coffee/reviews.coffee new file mode 100644 index 00000000..9667b171 --- /dev/null +++ b/public/coffee/reviews.coffee @@ -0,0 +1,23 @@ + +window.Reviews = + init: -> + $(".review .delete").live "click", (e) => @onReviewComplete e + + onReviewComplete: (e) -> + target = $(e.currentTarget) + repo = target.data("repo") + sha = target.data("sha") + $.ajax({ + type: "post", + url: "/complete_review_request", + data: { + repo_name: repo + commit_sha: sha + } + success: => + row = target.parents(".reviewRequestRow").detach() + $("#recentlyReviewed > tbody").prepend(row) + }) + false + +$(document).ready(-> Reviews.init()) diff --git a/public/css/saved_search.scss b/public/css/saved_search.scss index b322a9f5..2f0fdfc0 100644 --- a/public/css/saved_search.scss +++ b/public/css/saved_search.scss @@ -171,6 +171,15 @@ } } +.review { + a.delete { + padding-right: 5px; + float: right; + color: red; + @include closeLinks; + } +} + /* The actual commit search box. */ #commitSearch { padding: 10px 8px; diff --git a/views/_review_request.erb b/views/_review_request.erb new file mode 100644 index 00000000..42bf8b44 --- /dev/null +++ b/views/_review_request.erb @@ -0,0 +1,38 @@ +<%# + A single table row for a code review request. + View arguments: + - commit: a grit commit. + %> +<% db_commit = MetaRepo.instance.db_commit(commit.repo_name, commit.sha) %> + + + + + + + + + + + diff --git a/views/reviews.erb b/views/reviews.erb index 80a27908..b922916b 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -1,15 +1,26 @@ +<%= js_bundle(:reviews_app_js) %> +
    Code review requests
    -
    + X + " + title="Approved" rel="tipsy"> +
    + +
    +     		+ <%= commit.author.to_s[0..25] %> + + <%= commit.id_abbrev %> + + <%= CGI::escapeHTML(commit.short_message) %> + <%= commit.date.to_pretty %><%= commit.repo_name %> + <% comment_count = db_commit.comment_count %> + <% if comment_count > 0 %> + "> + <%= comment_count %>" /> + + <% end %> +
    +
    <% commits_with_uncompleted_reviews.each do |commit| %> - <%= erb :_unresolved_commit, :layout => false, :locals => { :commit => commit } %> + <%= erb :_review_request, :layout => false, :locals => { :commit => commit } %> <% end %> -
    + +
    + +
    +
    Recently completed code review requests
    + + <% recently_reviewed_commits.each do |commit| %> + <%= erb :_review_request, :layout => false, :locals => { :commit => commit } %> + <% end %> +
    Commits with unresolved comments
    - +
    <% commits_with_unresolved_comments.each do |commit| %> <%= erb :_unresolved_commit, :layout => false, :locals => { :commit => commit } %> <% end %> From 3b0f969210b4f31a12684327d095df5ea46d93ec Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sat, 19 Jan 2013 10:58:04 -0800 Subject: [PATCH 24/71] Add draggable handles to the review lists. --- public/coffee/reviews.coffee | 8 ++++++++ views/reviews.erb | 17 ++++++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/public/coffee/reviews.coffee b/public/coffee/reviews.coffee index 9667b171..c6ea7931 100644 --- a/public/coffee/reviews.coffee +++ b/public/coffee/reviews.coffee @@ -2,6 +2,14 @@ window.Reviews = init: -> $(".review .delete").live "click", (e) => @onReviewComplete e + $("#reviewLists").sortable + placeholder: "savedSearchPlaceholder" + handle: ".dragHandle" + axis: "y" + start: => + $.fn.tipsy.disable() + stop: => + $.fn.tipsy.enable() onReviewComplete: (e) -> target = $(e.currentTarget) diff --git a/views/reviews.erb b/views/reviews.erb index b922916b..4a21f31f 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -1,7 +1,11 @@ <%= js_bundle(:reviews_app_js) %> +
    -
    Code review requests
    +
    +
    +
    Code review requests
    +
    <% commits_with_uncompleted_reviews.each do |commit| %> <%= erb :_review_request, :layout => false, :locals => { :commit => commit } %> @@ -10,7 +14,10 @@
    -
    Recently completed code review requests
    +
    +
    +
    Recently completed code review requests
    +
    <% recently_reviewed_commits.each do |commit| %> <%= erb :_review_request, :layout => false, :locals => { :commit => commit } %> @@ -19,10 +26,14 @@
    -
    Commits with unresolved comments
    +
    +
    +
    Commits with unresolved comments
    +
    <% commits_with_unresolved_comments.each do |commit| %> <%= erb :_unresolved_commit, :layout => false, :locals => { :commit => commit } %> <% end %>
    +
    From 1d137c4d180ea911c397ebd5dc5950b39a8eb0e3 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sat, 19 Jan 2013 21:14:53 -0800 Subject: [PATCH 25/71] Add review_list_order to users table to keep track of list order preference. --- ...20130119143148_add_review_list_order_to_users.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 migrations/20130119143148_add_review_list_order_to_users.rb diff --git a/migrations/20130119143148_add_review_list_order_to_users.rb b/migrations/20130119143148_add_review_list_order_to_users.rb new file mode 100644 index 00000000..22b6b01a --- /dev/null +++ b/migrations/20130119143148_add_review_list_order_to_users.rb @@ -0,0 +1,13 @@ +require "bundler/setup" +require "pathological" +require "migrations/migration_helper.rb" + +Sequel.migration do + up do + add_column :users, :review_list_order, String + end + + down do + drop_column :users, :review_list_order + end +end From 6c9f9c5f1559bba42c85db94fb903b8331fcceb8 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sat, 19 Jan 2013 21:15:46 -0800 Subject: [PATCH 26/71] Keep track of review list order. --- barkeep_server.rb | 12 ++++++- public/coffee/reviews.coffee | 9 +++++ views/reviews.erb | 70 ++++++++++++++++++++---------------- 3 files changed, 60 insertions(+), 31 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index fcac378b..c8a30c61 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -502,13 +502,23 @@ def ensure_required_params(*required_params) commits_with_unresolved_comments = Commit.commits_with_unresolved_comments(current_user.email) commits_with_uncompleted_reviews = ReviewRequest.commits_with_uncompleted_reviews(current_user.id) recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) + default_list = "uncompleted_reviews,recent_reviews,unresolved_comments" + review_list_order = (current_user.review_list_order || default_list).split(",") erb :reviews, :locals => { :commits_with_unresolved_comments => commits_with_unresolved_comments, :commits_with_uncompleted_reviews => commits_with_uncompleted_reviews, - :recently_reviewed_commits => recently_reviewed_commits, + :recently_reviewed_commits => recently_reviewed_commits, + :review_list_ids => review_list_order, } end + post "/review_lists/reorder" do + review_list_ids = request.body.read + User.filter(:id => current_user.id). + update(:review_list_order => review_list_ids) + nil + end + get "/profile/:id" do user = User[params[:id]] halt 404 unless user diff --git a/public/coffee/reviews.coffee b/public/coffee/reviews.coffee index c6ea7931..50a36509 100644 --- a/public/coffee/reviews.coffee +++ b/public/coffee/reviews.coffee @@ -10,6 +10,15 @@ window.Reviews = $.fn.tipsy.disable() stop: => $.fn.tipsy.enable() + @reorderReviewLists() + + reorderReviewLists: -> + state = for reviewList in $("#reviewLists .review") + $(reviewList).data("review-list-id") + $.ajax + type: "POST" + url: "/review_lists/reorder" + data: state.toString() onReviewComplete: (e) -> target = $(e.currentTarget) diff --git a/views/reviews.erb b/views/reviews.erb index 4a21f31f..c7ff0d44 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -1,39 +1,49 @@ <%= js_bundle(:reviews_app_js) %>
    -
    -
    -
    -
    Code review requests
    +<% review_list_ids.each do |review_list_id| %> + +<% if review_list_id == "uncompleted_reviews" %> +
    +
    +
    +
    Code review requests
    +
    + + <% commits_with_uncompleted_reviews.each do |commit| %> + <%= erb :_review_request, :layout => false, :locals => { :commit => commit } %> + <% end %> +
    - - <% commits_with_uncompleted_reviews.each do |commit| %> - <%= erb :_review_request, :layout => false, :locals => { :commit => commit } %> - <% end %> -
    -
    +<% end %> -
    -
    -
    -
    Recently completed code review requests
    +<% if review_list_id == "recent_reviews" %> +
    +
    +
    +
    Recently completed code review requests
    +
    + + <% recently_reviewed_commits.each do |commit| %> + <%= erb :_review_request, :layout => false, :locals => { :commit => commit } %> + <% end %> +
    - - <% recently_reviewed_commits.each do |commit| %> - <%= erb :_review_request, :layout => false, :locals => { :commit => commit } %> - <% end %> -
    -
    +<% end %> -
    -
    -
    -
    Commits with unresolved comments
    +<% if review_list_id == "unresolved_comments" %> +
    +
    +
    +
    Commits with unresolved comments
    +
    + + <% commits_with_unresolved_comments.each do |commit| %> + <%= erb :_unresolved_commit, :layout => false, :locals => { :commit => commit } %> + <% end %> +
    - - <% commits_with_unresolved_comments.each do |commit| %> - <%= erb :_unresolved_commit, :layout => false, :locals => { :commit => commit } %> - <% end %> -
    -
    +<% end %> + +<% end %>
    From 7831667fd4be75f7ad01323fec59ef7d3b517c6b Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sat, 19 Jan 2013 22:28:20 -0800 Subject: [PATCH 27/71] Refactor models/review_request.rb and add requests_from_me(). --- models/review_request.rb | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/models/review_request.rb b/models/review_request.rb index c1965bf1..351cb54e 100644 --- a/models/review_request.rb +++ b/models/review_request.rb @@ -3,16 +3,19 @@ class ReviewRequest < Sequel::Model many_to_one :requester_user, :class => User many_to_one :reviewer_user, :class => User - def self.commits_with_uncompleted_reviews(user_id) - uncompleted = ReviewRequest.filter(:reviewer_user_id => user_id, :completed_at => nil). - group_by(:commit_id).all - commits = uncompleted.map do |review| + def self.get_grit_commits(reviews) + grit_commits = reviews.map do |review| grit_commit = MetaRepo.instance.grit_commit(review.commit.git_repo.name, review.commit.sha) next unless grit_commit grit_commit end - commits.reject!(&:nil?) - commits + grit_commits.reject(&:nil?) + end + + def self.commits_with_uncompleted_reviews(user_id) + uncompleted = ReviewRequest.filter(:reviewer_user_id => user_id, :completed_at => nil). + group_by(:commit_id).all + get_grit_commits(uncompleted) end def self.recently_reviewed_commits(user_id) @@ -20,13 +23,14 @@ def self.recently_reviewed_commits(user_id) exclude(:completed_at => nil). group_by(:commit_id). reverse_order(:completed_at).limit(5).all - commits = recently_reviewed.map do |review| - grit_commit = MetaRepo.instance.grit_commit(review.commit.git_repo.name, review.commit.sha) - next unless grit_commit - grit_commit - end - commits.reject!(&:nil?) - commits + get_grit_commits(recently_reviewed) + end + + def self.requests_from_me(user_id) + p ReviewRequest.filter(:requester_user_id => user_id, :completed_at => nil).group_by(:commit_id).sql + uncompleted = ReviewRequest.filter(:requester_user_id => user_id, :completed_at => nil). + group_by(:commit_id).all + get_grit_commits(uncompleted) end def self.complete_requests(commit_id) From 671640ef317c4546bf1fef271cc783d8d9b246da Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sat, 19 Jan 2013 22:30:30 -0800 Subject: [PATCH 28/71] Refactor views/reviews.erb and add 'requests from me' list. --- barkeep_server.rb | 4 ++- public/coffee/reviews.coffee | 4 +-- views/_review_list.erb | 16 ++++++++++ views/reviews.erb | 60 ++++++++++++++---------------------- 4 files changed, 44 insertions(+), 40 deletions(-) create mode 100644 views/_review_list.erb diff --git a/barkeep_server.rb b/barkeep_server.rb index c8a30c61..cce32289 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -502,12 +502,14 @@ def ensure_required_params(*required_params) commits_with_unresolved_comments = Commit.commits_with_unresolved_comments(current_user.email) commits_with_uncompleted_reviews = ReviewRequest.commits_with_uncompleted_reviews(current_user.id) recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) - default_list = "uncompleted_reviews,recent_reviews,unresolved_comments" + requests_from_me = ReviewRequest.requests_from_me(current_user.id) + default_list = "uncompleted_reviews,recent_reviews,unresolved_comments,requests_from_me" review_list_order = (current_user.review_list_order || default_list).split(",") erb :reviews, :locals => { :commits_with_unresolved_comments => commits_with_unresolved_comments, :commits_with_uncompleted_reviews => commits_with_uncompleted_reviews, :recently_reviewed_commits => recently_reviewed_commits, + :requests_from_me => requests_from_me, :review_list_ids => review_list_order, } end diff --git a/public/coffee/reviews.coffee b/public/coffee/reviews.coffee index 50a36509..b1c5cfc6 100644 --- a/public/coffee/reviews.coffee +++ b/public/coffee/reviews.coffee @@ -14,7 +14,7 @@ window.Reviews = reorderReviewLists: -> state = for reviewList in $("#reviewLists .review") - $(reviewList).data("review-list-id") + $(reviewList).data("list-id") $.ajax type: "POST" url: "/review_lists/reorder" @@ -33,7 +33,7 @@ window.Reviews = } success: => row = target.parents(".reviewRequestRow").detach() - $("#recentlyReviewed > tbody").prepend(row) + $("#recent_reviewsTable > tbody").prepend(row) }) false diff --git a/views/_review_list.erb b/views/_review_list.erb new file mode 100644 index 00000000..7930096c --- /dev/null +++ b/views/_review_list.erb @@ -0,0 +1,16 @@ + +
    +
    +
    +
    <%= header %>
    +
    + <% if requests.empty? %> +
    None.
    + <% else %> + + <% requests.each do |commit| %> + <%= erb erb_file, :layout => false, :locals => { :commit => commit } %> + <% end %> +
    + <% end %> +
    diff --git a/views/reviews.erb b/views/reviews.erb index c7ff0d44..93c5d315 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -1,48 +1,34 @@ <%= js_bundle(:reviews_app_js) %>
    -<% review_list_ids.each do |review_list_id| %> +<% review_list_ids.each do |list_id| %> -<% if review_list_id == "uncompleted_reviews" %> -
    -
    -
    -
    Code review requests
    -
    - - <% commits_with_uncompleted_reviews.each do |commit| %> - <%= erb :_review_request, :layout => false, :locals => { :commit => commit } %> - <% end %> -
    -
    +<% if list_id == "uncompleted_reviews" %> + <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + :header => "For me: code review requests", + :requests => commits_with_uncompleted_reviews, + :erb_file => :_review_request } %> <% end %> -<% if review_list_id == "recent_reviews" %> -
    -
    -
    -
    Recently completed code review requests
    -
    - - <% recently_reviewed_commits.each do |commit| %> - <%= erb :_review_request, :layout => false, :locals => { :commit => commit } %> - <% end %> -
    -
    +<% if list_id == "recent_reviews" %> + <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + :header => "My recently completed code review requests", + :requests => recently_reviewed_commits, + :erb_file => :_review_request } %> <% end %> -<% if review_list_id == "unresolved_comments" %> -
    -
    -
    -
    Commits with unresolved comments
    -
    - - <% commits_with_unresolved_comments.each do |commit| %> - <%= erb :_unresolved_commit, :layout => false, :locals => { :commit => commit } %> - <% end %> -
    -
    +<% if list_id == "unresolved_comments" %> + <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + :header => "For me: commits with unresolved comments", + :requests => commits_with_unresolved_comments, + :erb_file => :_unresolved_commit } %> +<% end %> + +<% if list_id == "requests_from_me" %> + <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + :header => "From me: code review requests", + :requests => requests_from_me, + :erb_file => :_review_request } %> <% end %> <% end %> From 85bad1cd29c524abc2dac14213e7e86fd0b668df Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sat, 19 Jan 2013 23:08:54 -0800 Subject: [PATCH 29/71] Show 'None' when a review list becomes empty. --- public/coffee/reviews.coffee | 7 ++++++- views/_review_list.erb | 5 ++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/public/coffee/reviews.coffee b/public/coffee/reviews.coffee index b1c5cfc6..0e4300a5 100644 --- a/public/coffee/reviews.coffee +++ b/public/coffee/reviews.coffee @@ -14,7 +14,7 @@ window.Reviews = reorderReviewLists: -> state = for reviewList in $("#reviewLists .review") - $(reviewList).data("list-id") + reviewList.id $.ajax type: "POST" url: "/review_lists/reorder" @@ -33,7 +33,12 @@ window.Reviews = } success: => row = target.parents(".reviewRequestRow").detach() + $("#recent_reviews .noResults").hide() + $("#recent_reviewsTable").show() $("#recent_reviewsTable > tbody").prepend(row) + if $("#uncompleted_reviewsTable > tbody tr").length == 0 + $("#uncompleted_reviewsTable").hide() + $("#uncompleted_reviews .noResults").show() }) false diff --git a/views/_review_list.erb b/views/_review_list.erb index 7930096c..6a1e1072 100644 --- a/views/_review_list.erb +++ b/views/_review_list.erb @@ -1,12 +1,15 @@ -
    +
    <%= header %>
    <% if requests.empty? %>
    None.
    + + <% else %> + <% requests.each do |commit| %> <%= erb erb_file, :layout => false, :locals => { :commit => commit } %> From 96cb6b9f6c5548669cb45bee61c990586b838378 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sat, 19 Jan 2013 23:14:44 -0800 Subject: [PATCH 30/71] Document the _review_list.erb view and outdent the code. No logic changes. --- views/_review_list.erb | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/views/_review_list.erb b/views/_review_list.erb index 6a1e1072..42e83c24 100644 --- a/views/_review_list.erb +++ b/views/_review_list.erb @@ -1,19 +1,27 @@ +<%# + This view snippet is used for a list of code review requests or a list of commits with unresolved comments. + View arguments: + - list_id: a string that identifies the list + - header: a string that is displayed above the list + - requests: an array of grit commits + - erb_file: the erb file to call for each line +%> -
    -
    -
    -
    <%= header %>
    -
    - <% if requests.empty? %> -
    None.
    -
    - - <% else %> - - - <% requests.each do |commit| %> - <%= erb erb_file, :layout => false, :locals => { :commit => commit } %> - <% end %> -
    - <% end %> +
    +
    +
    +
    <%= header %>
    + <% if requests.empty? %> +
    None.
    + + + <% else %> + + + <% requests.each do |commit| %> + <%= erb erb_file, :layout => false, :locals => { :commit => commit } %> + <% end %> +
    + <% end %> +
    From a92009af7ad472ef92ebd8d369b8ea864de269db Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 20 Jan 2013 15:37:35 -0800 Subject: [PATCH 31/71] Show the 'delete' icon (red X) only for uncompleted review requests. --- public/css/saved_search.scss | 4 +++- views/_review_request.erb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/public/css/saved_search.scss b/public/css/saved_search.scss index 2f0fdfc0..4719c42b 100644 --- a/public/css/saved_search.scss +++ b/public/css/saved_search.scss @@ -171,13 +171,15 @@ } } -.review { +#reviewLists { a.delete { padding-right: 5px; float: right; color: red; @include closeLinks; } + .tableCellDelete { visibility: hidden; } + #uncompleted_reviews .tableCellDelete { visibility: visible; } } /* The actual commit search box. */ diff --git a/views/_review_request.erb b/views/_review_request.erb index 42bf8b44..2071b02f 100644 --- a/views/_review_request.erb +++ b/views/_review_request.erb @@ -5,7 +5,7 @@ %> <% db_commit = MetaRepo.instance.db_commit(commit.repo_name, commit.sha) %> - + X " From 95c91ff9f4e88208ea4ec5b0894258b8a17057c3 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 20 Jan 2013 15:42:16 -0800 Subject: [PATCH 32/71] Add a hidden table cell to the unresolved comments list to make it indent like other lists. --- views/_unresolved_commit.erb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/views/_unresolved_commit.erb b/views/_unresolved_commit.erb index 351fb3d6..69891764 100644 --- a/views/_unresolved_commit.erb +++ b/views/_unresolved_commit.erb @@ -5,6 +5,9 @@ %> <% db_commit = MetaRepo.instance.db_commit(commit.repo_name, commit.sha) %> + + X + " title="Approved" rel="tipsy"> From 16ae97a6b23125f224f3d95a876323788d53cc92 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 20 Jan 2013 16:00:45 -0800 Subject: [PATCH 33/71] Add 'recently resolved comments' list to 'reviews' view. --- barkeep_server.rb | 4 +++- models/commit.rb | 29 +++++++++++++++++++++++------ views/reviews.erb | 7 +++++++ 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index cce32289..a1a9f51c 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -500,14 +500,16 @@ def ensure_required_params(*required_params) get "/reviews" do commits_with_unresolved_comments = Commit.commits_with_unresolved_comments(current_user.email) + recently_resolved_comments = Commit.commits_with_recently_resolved_comments(current_user.email) commits_with_uncompleted_reviews = ReviewRequest.commits_with_uncompleted_reviews(current_user.id) recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) requests_from_me = ReviewRequest.requests_from_me(current_user.id) - default_list = "uncompleted_reviews,recent_reviews,unresolved_comments,requests_from_me" + default_list = "uncompleted_reviews,recent_reviews,unresolved_comments,recent_comments,requests_from_me" review_list_order = (current_user.review_list_order || default_list).split(",") erb :reviews, :locals => { :commits_with_unresolved_comments => commits_with_unresolved_comments, :commits_with_uncompleted_reviews => commits_with_uncompleted_reviews, + :recently_resolved_comments => recently_resolved_comments, :recently_reviewed_commits => recently_reviewed_commits, :requests_from_me => requests_from_me, :review_list_ids => review_list_order, diff --git a/models/commit.rb b/models/commit.rb index 93f724d4..764177ad 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -72,6 +72,15 @@ def self.prefix_match(git_repo, partial_sha, zero_commits_ok = false) end end + def self.get_grit_commits(commits) + grit_commits = commits.map do |commit| + grit_commit = MetaRepo.instance.grit_commit(commit.git_repo.name, commit.sha) + next unless grit_commit + grit_commit + end + grit_commits.reject(&:nil?) + end + # Fetches the commits with unresolved comments for the given email. The email is used to find # the commits by that user and to exclude comments made by that user. def self.commits_with_unresolved_comments(email) @@ -82,11 +91,19 @@ def self.commits_with_unresolved_comments(email) filter(:authors__email => email, :comments__completed_at => nil). exclude(:users__email => email). group_by(:commits__id).all - commits.map! do |commit| - grit_commit = MetaRepo.instance.grit_commit(commit.git_repo.name, commit.sha) - next unless grit_commit - grit_commit - end - commits.reject(&:nil?) + get_grit_commits(commits) + end + + def self.commits_with_recently_resolved_comments(email) + commits = Commit. + join(:comments, :commit_id => :id). + join(:authors, :id => :commits__author_id). + join(:users, :id => :comments__user_id). + filter(:authors__email => email). + exclude(:comments__completed_at => nil). + exclude(:users__email => email). + group_by(:commits__id). + reverse_order(:completed_at).limit(5).all + get_grit_commits(commits) end end diff --git a/views/reviews.erb b/views/reviews.erb index 93c5d315..869d193d 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -24,6 +24,13 @@ :erb_file => :_unresolved_commit } %> <% end %> +<% if list_id == "recent_comments" %> + <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + :header => "My commits with recently resolved comments", + :requests => recently_resolved_comments, + :erb_file => :_unresolved_commit } %> +<% end %> + <% if list_id == "requests_from_me" %> <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, :header => "From me: code review requests", From bc9a4b6bf3c87698717e130e00f38681f8f78c7e Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 20 Jan 2013 16:10:10 -0800 Subject: [PATCH 34/71] Add 'commits with unresolved comments from me' to 'Reviews' view. --- barkeep_server.rb | 4 +++- models/commit.rb | 11 +++++++++++ views/reviews.erb | 7 +++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index a1a9f51c..57124e76 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -501,10 +501,11 @@ def ensure_required_params(*required_params) get "/reviews" do commits_with_unresolved_comments = Commit.commits_with_unresolved_comments(current_user.email) recently_resolved_comments = Commit.commits_with_recently_resolved_comments(current_user.email) + comments_from_me = Commit.commits_with_unresolved_comments_from_me(current_user.email) commits_with_uncompleted_reviews = ReviewRequest.commits_with_uncompleted_reviews(current_user.id) recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) requests_from_me = ReviewRequest.requests_from_me(current_user.id) - default_list = "uncompleted_reviews,recent_reviews,unresolved_comments,recent_comments,requests_from_me" + default_list = "uncompleted_reviews,recent_reviews,unresolved_comments,recent_comments,requests_from_me,comments_from_me" review_list_order = (current_user.review_list_order || default_list).split(",") erb :reviews, :locals => { :commits_with_unresolved_comments => commits_with_unresolved_comments, @@ -512,6 +513,7 @@ def ensure_required_params(*required_params) :recently_resolved_comments => recently_resolved_comments, :recently_reviewed_commits => recently_reviewed_commits, :requests_from_me => requests_from_me, + :comments_from_me => comments_from_me, :review_list_ids => review_list_order, } end diff --git a/models/commit.rb b/models/commit.rb index 764177ad..cbb825ac 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -106,4 +106,15 @@ def self.commits_with_recently_resolved_comments(email) reverse_order(:completed_at).limit(5).all get_grit_commits(commits) end + + def self.commits_with_unresolved_comments_from_me(email) + commits = Commit. + join(:comments, :commit_id => :id). + join(:authors, :id => :commits__author_id). + join(:users, :id => :comments__user_id). + filter(:users__email => email, :comments__completed_at => nil). + exclude(:authors__email => email). + group_by(:commits__id).all + get_grit_commits(commits) + end end diff --git a/views/reviews.erb b/views/reviews.erb index 869d193d..16c496d9 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -38,5 +38,12 @@ :erb_file => :_review_request } %> <% end %> +<% if list_id == "comments_from_me" %> + <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + :header => "From me: commits with unresolved comments", + :requests => comments_from_me, + :erb_file => :_unresolved_commit } %> +<% end %> + <% end %>
    From d0901c626ad20fe127805e115356f66c07b19b9c Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 20 Jan 2013 20:10:34 -0800 Subject: [PATCH 35/71] Remove debugging. --- models/review_request.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/models/review_request.rb b/models/review_request.rb index 351cb54e..88c938c9 100644 --- a/models/review_request.rb +++ b/models/review_request.rb @@ -27,7 +27,6 @@ def self.recently_reviewed_commits(user_id) end def self.requests_from_me(user_id) - p ReviewRequest.filter(:requester_user_id => user_id, :completed_at => nil).group_by(:commit_id).sql uncompleted = ReviewRequest.filter(:requester_user_id => user_id, :completed_at => nil). group_by(:commit_id).all get_grit_commits(uncompleted) From 052909e48e3b35e54a390da53151fd7002850492 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 20 Jan 2013 21:20:21 -0800 Subject: [PATCH 36/71] Fix comment in lib/models.rb to replace irrelevant example with a relevant one. No code changes. --- lib/models.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/models.rb b/lib/models.rb index b47e7017..6ea061e6 100644 --- a/lib/models.rb +++ b/lib/models.rb @@ -5,7 +5,7 @@ :password => DB_PASSWORD, :adapter => "mysql2") # This plugin gives you the "add_association_dependency" method, which lets you specify other objects to be -# destroyed when the current model gets destroyed, e.g. when you delete a provider, also delete its movies. +# destroyed when the current model gets destroyed, e.g. when you delete a git repo, also delete its commits. Sequel::Model.plugin :association_dependencies # Auto-populate "created_at" and "updated_at" fields. From 8901f7c9e6520acb745ede07ef7c5ff7f82a0513 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 21 Jan 2013 07:01:26 -0800 Subject: [PATCH 37/71] Refactored the views for 'review requests' and 'commits with comments' so they can evolve independently. --- lib/models.rb | 1 + lib/review_list_entry.rb | 11 +++++++ models/commit.rb | 15 +++++----- models/review_request.rb | 17 ++++++----- ...it.erb => _commit_with_comments_entry.erb} | 3 +- views/_commits_with_comments_list.erb | 26 ++++++++++++++++ ..._request.erb => _review_request_entry.erb} | 3 +- ...view_list.erb => _review_request_list.erb} | 9 +++--- views/reviews.erb | 30 ++++++++----------- 9 files changed, 76 insertions(+), 39 deletions(-) create mode 100644 lib/review_list_entry.rb rename views/{_unresolved_commit.erb => _commit_with_comments_entry.erb} (95%) create mode 100644 views/_commits_with_comments_list.erb rename views/{_review_request.erb => _review_request_entry.erb} (96%) rename views/{_review_list.erb => _review_request_list.erb} (75%) diff --git a/lib/models.rb b/lib/models.rb index 6ea061e6..fb4b633a 100644 --- a/lib/models.rb +++ b/lib/models.rb @@ -21,3 +21,4 @@ require "models/comment" require "models/completed_email" require "models/review_request" +require "lib/review_list_entry" diff --git a/lib/review_list_entry.rb b/lib/review_list_entry.rb new file mode 100644 index 00000000..da6946cf --- /dev/null +++ b/lib/review_list_entry.rb @@ -0,0 +1,11 @@ +# This class encapsulates the data for an entry in a "review list" on the "Reviews" page. + +class ReviewListEntry + attr_accessor :grit_commit + attr_accessor :review_request + + def initialize(grit_commit) + @grit_commit = grit_commit + @review_request = nil + end +end diff --git a/models/commit.rb b/models/commit.rb index cbb825ac..9b618d91 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -72,13 +72,14 @@ def self.prefix_match(git_repo, partial_sha, zero_commits_ok = false) end end - def self.get_grit_commits(commits) - grit_commits = commits.map do |commit| + def self.create_review_list_entries(commits) + entries = [] + commits.each do |commit| grit_commit = MetaRepo.instance.grit_commit(commit.git_repo.name, commit.sha) next unless grit_commit - grit_commit + entries << ReviewListEntry.new(grit_commit) end - grit_commits.reject(&:nil?) + entries end # Fetches the commits with unresolved comments for the given email. The email is used to find @@ -91,7 +92,7 @@ def self.commits_with_unresolved_comments(email) filter(:authors__email => email, :comments__completed_at => nil). exclude(:users__email => email). group_by(:commits__id).all - get_grit_commits(commits) + create_review_list_entries(commits) end def self.commits_with_recently_resolved_comments(email) @@ -104,7 +105,7 @@ def self.commits_with_recently_resolved_comments(email) exclude(:users__email => email). group_by(:commits__id). reverse_order(:completed_at).limit(5).all - get_grit_commits(commits) + create_review_list_entries(commits) end def self.commits_with_unresolved_comments_from_me(email) @@ -115,6 +116,6 @@ def self.commits_with_unresolved_comments_from_me(email) filter(:users__email => email, :comments__completed_at => nil). exclude(:authors__email => email). group_by(:commits__id).all - get_grit_commits(commits) + create_review_list_entries(commits) end end diff --git a/models/review_request.rb b/models/review_request.rb index 88c938c9..2455efd0 100644 --- a/models/review_request.rb +++ b/models/review_request.rb @@ -3,19 +3,22 @@ class ReviewRequest < Sequel::Model many_to_one :requester_user, :class => User many_to_one :reviewer_user, :class => User - def self.get_grit_commits(reviews) - grit_commits = reviews.map do |review| + def self.create_review_list_entries(reviews) + entries = [] + reviews.each do |review| grit_commit = MetaRepo.instance.grit_commit(review.commit.git_repo.name, review.commit.sha) next unless grit_commit - grit_commit + entry = ReviewListEntry.new(grit_commit) + entry.review_request = review + entries << entry end - grit_commits.reject(&:nil?) + entries end def self.commits_with_uncompleted_reviews(user_id) uncompleted = ReviewRequest.filter(:reviewer_user_id => user_id, :completed_at => nil). group_by(:commit_id).all - get_grit_commits(uncompleted) + create_review_list_entries(uncompleted) end def self.recently_reviewed_commits(user_id) @@ -23,13 +26,13 @@ def self.recently_reviewed_commits(user_id) exclude(:completed_at => nil). group_by(:commit_id). reverse_order(:completed_at).limit(5).all - get_grit_commits(recently_reviewed) + create_review_list_entries(recently_reviewed) end def self.requests_from_me(user_id) uncompleted = ReviewRequest.filter(:requester_user_id => user_id, :completed_at => nil). group_by(:commit_id).all - get_grit_commits(uncompleted) + create_review_list_entries(uncompleted) end def self.complete_requests(commit_id) diff --git a/views/_unresolved_commit.erb b/views/_commit_with_comments_entry.erb similarity index 95% rename from views/_unresolved_commit.erb rename to views/_commit_with_comments_entry.erb index 69891764..66bb4d81 100644 --- a/views/_unresolved_commit.erb +++ b/views/_commit_with_comments_entry.erb @@ -1,8 +1,9 @@ <%# A single table row for a commit with unresolved comments. View arguments: - - commit: a grit commit. + - entry: a ReviewListEntry %> +<% commit = entry.grit_commit %> <% db_commit = MetaRepo.instance.db_commit(commit.repo_name, commit.sha) %> diff --git a/views/_commits_with_comments_list.erb b/views/_commits_with_comments_list.erb new file mode 100644 index 00000000..6487721e --- /dev/null +++ b/views/_commits_with_comments_list.erb @@ -0,0 +1,26 @@ +<%# + This view snippet is used for a list of code review requests or a list of commits with unresolved comments. + View arguments: + - list_id: a string that identifies the list + - header: a string that is displayed above the list + - review_list: an array where each element is a ReviewListEntry +%> + +
    +
    +
    +
    <%= header %>
    +
    + <% if review_list.empty? %> +
    None.
    + + + <% else %> + + + <% review_list.each do |entry| %> + <%= erb :_commit_with_comments_entry, :layout => false, :locals => { :entry => entry } %> + <% end %> +
    + <% end %> +
    diff --git a/views/_review_request.erb b/views/_review_request_entry.erb similarity index 96% rename from views/_review_request.erb rename to views/_review_request_entry.erb index 2071b02f..c8e78971 100644 --- a/views/_review_request.erb +++ b/views/_review_request_entry.erb @@ -1,8 +1,9 @@ <%# A single table row for a code review request. View arguments: - - commit: a grit commit. + - entry: a ReviewListEntry %> +<% commit = entry.grit_commit %> <% db_commit = MetaRepo.instance.db_commit(commit.repo_name, commit.sha) %> diff --git a/views/_review_list.erb b/views/_review_request_list.erb similarity index 75% rename from views/_review_list.erb rename to views/_review_request_list.erb index 42e83c24..ac8d6a7e 100644 --- a/views/_review_list.erb +++ b/views/_review_request_list.erb @@ -3,8 +3,7 @@ View arguments: - list_id: a string that identifies the list - header: a string that is displayed above the list - - requests: an array of grit commits - - erb_file: the erb file to call for each line + - review_list: an array where each element is a ReviewListEntry %>
    @@ -12,15 +11,15 @@
    <%= header %>
    - <% if requests.empty? %> + <% if review_list.empty? %>
    None.
    <% else %> - <% requests.each do |commit| %> - <%= erb erb_file, :layout => false, :locals => { :commit => commit } %> + <% review_list.each do |entry| %> + <%= erb :_review_request_entry, :layout => false, :locals => { :entry => entry } %> <% end %>
    <% end %> diff --git a/views/reviews.erb b/views/reviews.erb index 16c496d9..56a56c2b 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -4,45 +4,39 @@ <% review_list_ids.each do |list_id| %> <% if list_id == "uncompleted_reviews" %> - <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, :header => "For me: code review requests", - :requests => commits_with_uncompleted_reviews, - :erb_file => :_review_request } %> + :review_list => commits_with_uncompleted_reviews } %> <% end %> <% if list_id == "recent_reviews" %> - <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, :header => "My recently completed code review requests", - :requests => recently_reviewed_commits, - :erb_file => :_review_request } %> + :review_list => recently_reviewed_commits } %> <% end %> <% if list_id == "unresolved_comments" %> - <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, :header => "For me: commits with unresolved comments", - :requests => commits_with_unresolved_comments, - :erb_file => :_unresolved_commit } %> + :review_list => commits_with_unresolved_comments } %> <% end %> <% if list_id == "recent_comments" %> - <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, :header => "My commits with recently resolved comments", - :requests => recently_resolved_comments, - :erb_file => :_unresolved_commit } %> + :review_list => recently_resolved_comments } %> <% end %> <% if list_id == "requests_from_me" %> - <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, :header => "From me: code review requests", - :requests => requests_from_me, - :erb_file => :_review_request } %> + :review_list => requests_from_me } %> <% end %> <% if list_id == "comments_from_me" %> - <%= erb :_review_list, :layout => false, :locals => { :list_id => list_id, + <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, :header => "From me: commits with unresolved comments", - :requests => comments_from_me, - :erb_file => :_unresolved_commit } %> + :review_list => comments_from_me } %> <% end %> <% end %> From 180b0fb5c6ae7bf5ec1706fd325649c7548f2a49 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 21 Jan 2013 10:06:40 -0800 Subject: [PATCH 38/71] Add 'Requested at' or 'Completed at' message to review request list. --- public/css/styles.scss | 65 +++++++++++++++++++++++++++++++++ views/_review_request_entry.erb | 55 ++++++++++++++++++---------- views/_review_request_list.erb | 9 ++++- views/reviews.erb | 3 ++ 4 files changed, 111 insertions(+), 21 deletions(-) diff --git a/public/css/styles.scss b/public/css/styles.scss index af6c9a6a..93d956e9 100644 --- a/public/css/styles.scss +++ b/public/css/styles.scss @@ -430,6 +430,71 @@ table.commitsList, table.authorList { .selected .selectArrow { background: url(/assets/images/right_arrow.png) no-repeat 0px 2px; } } +table.innerCommitsList { + border-collapse: collapse; + border-width: 0px; + + font-size: 14px; + a { + position: relative; + text-decoration: none; + color: $textColor; + border: 0; + } + td { + padding-right: 7px; + a { + &:active { background-color: transparent; } + &:hover { text-decoration: underline; } + } + &.commitId { + font: 16px $monoFont; + a { color: $lightTextColor; } + text-align: right; + padding-right: 7px; + } + &.commitMessage { + width: 100%; + a { + display: block; + width: 100%; + /* A height with overflow implements truncation of commit messages. There may be second and third + * lines of text in this element, but they'll be hidden. */ + overflow: hidden; + height: 1.2em; + } + } + &.commitLine2 { + font-size: 12px; + font-style: italic; + } + &.commitDate { + font-size: 13px; + text-align: right; + } + &.commitDate, &.commitRepo { + white-space: nowrap; + color: $lightTextColor; + } + &.commitRepo { + text-transform: uppercase; + font-size: 10px; + padding-right: 5px; + } + &.commentCount { + min-width: 25px; + padding-bottom: 2px; + white-space: nowrap; + text-align: right; + a { @include noLink; } + span { + font-size: 12px; + color: $lightTextColor; + } + } + } +} + /* * Styles for the stats page. diff --git a/views/_review_request_entry.erb b/views/_review_request_entry.erb index c8e78971..67298335 100644 --- a/views/_review_request_entry.erb +++ b/views/_review_request_entry.erb @@ -2,6 +2,7 @@ A single table row for a code review request. View arguments: - entry: a ReviewListEntry + - request_message: the "requested at" or "completed at" message %> <% commit = entry.grit_commit %> <% db_commit = MetaRepo.instance.db_commit(commit.repo_name, commit.sha) %> @@ -16,24 +17,38 @@
    - - <%= commit.author.to_s[0..25] %> - - - <%= commit.id_abbrev %> - - - <%= CGI::escapeHTML(commit.short_message) %> - - <%= commit.date.to_pretty %> - <%= commit.repo_name %> - - <% comment_count = db_commit.comment_count %> - <% if comment_count > 0 %> - "> - <%= comment_count %>" /> - - <% end %> - + + + + + +
    + <%= commit.author.to_s[0..25] %> +
     
    + + + + + + + + + + + +
    + <%= commit.id_abbrev %> + + <%= CGI::escapeHTML(commit.short_message) %> + <%= commit.date.to_pretty %><%= commit.repo_name %> + <% comment_count = db_commit.comment_count %> + <% if comment_count > 0 %> + "> + <%= comment_count %>" /> + + <% end %> +
    + <%= request_message %> +
    diff --git a/views/_review_request_list.erb b/views/_review_request_list.erb index ac8d6a7e..9ece92d8 100644 --- a/views/_review_request_list.erb +++ b/views/_review_request_list.erb @@ -4,6 +4,7 @@ - list_id: a string that identifies the list - header: a string that is displayed above the list - review_list: an array where each element is a ReviewListEntry + - request_type: either "Requested" or "Completed" %>
    @@ -19,7 +20,13 @@ <% review_list.each do |entry| %> - <%= erb :_review_request_entry, :layout => false, :locals => { :entry => entry } %> + <% if request_type == "Requested" %> + <% request_message = "Requested #{entry.review_request.requested_at.to_pretty}" %> + <% else %> + <% request_message = "Completed #{entry.review_request.completed_at.to_pretty}" %> + <% end %> + <%= erb :_review_request_entry, :layout => false, :locals => { :entry => entry, + :request_message => request_message } %> <% end %>
    <% end %> diff --git a/views/reviews.erb b/views/reviews.erb index 56a56c2b..3899c8fc 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -6,12 +6,14 @@ <% if list_id == "uncompleted_reviews" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, :header => "For me: code review requests", + :request_type => "Requested", :review_list => commits_with_uncompleted_reviews } %> <% end %> <% if list_id == "recent_reviews" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, :header => "My recently completed code review requests", + :request_type => "Completed", :review_list => recently_reviewed_commits } %> <% end %> @@ -30,6 +32,7 @@ <% if list_id == "requests_from_me" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, :header => "From me: code review requests", + :request_type => "Requested", :review_list => requests_from_me } %> <% end %> From aa380cdf6dad1e53838f09f39548cecbb3032598 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 21 Jan 2013 10:40:30 -0800 Subject: [PATCH 39/71] Use server to generate 'recently completed reviews' list after completing a review. --- barkeep_server.rb | 9 ++++++++- public/coffee/reviews.coffee | 8 +++----- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index 57124e76..9f8e2390 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -390,7 +390,14 @@ def ensure_required_params(*required_params) commit = MetaRepo.instance.db_commit(params[:repo_name], params[:commit_sha]) halt 400 unless commit ReviewRequest.complete_requests(commit.id) - nil + recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) + erb :_review_request_list, :layout => false, :locals => { + :list_id => "recent_reviews", + :recently_reviewed_commits => recently_reviewed_commits, + :header => "My recently completed code review requests", + :request_type => "Completed", + :review_list => recently_reviewed_commits + } end # Saves changes to the user-level search options. diff --git a/public/coffee/reviews.coffee b/public/coffee/reviews.coffee index 0e4300a5..df8811de 100644 --- a/public/coffee/reviews.coffee +++ b/public/coffee/reviews.coffee @@ -31,14 +31,12 @@ window.Reviews = repo_name: repo commit_sha: sha } - success: => - row = target.parents(".reviewRequestRow").detach() - $("#recent_reviews .noResults").hide() - $("#recent_reviewsTable").show() - $("#recent_reviewsTable > tbody").prepend(row) + success: (html) => + target.parents(".reviewRequestRow").remove() if $("#uncompleted_reviewsTable > tbody tr").length == 0 $("#uncompleted_reviewsTable").hide() $("#uncompleted_reviews .noResults").show() + $("#recent_reviews").html(html) }) false From 75a407728944c3230272f8d82e1a92f0074f1363 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 21 Jan 2013 17:03:17 -0800 Subject: [PATCH 40/71] Add highlighted labels to the lists on the Reviews page. --- barkeep_server.rb | 4 +++- public/css/saved_search.scss | 5 +++++ views/_commits_with_comments_list.erb | 6 +++++- views/_review_request_list.erb | 7 ++++++- views/reviews.erb | 24 ++++++++++++++++++------ 5 files changed, 37 insertions(+), 9 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index 9f8e2390..bbc0cd27 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -393,6 +393,8 @@ def ensure_required_params(*required_params) recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) erb :_review_request_list, :layout => false, :locals => { :list_id => "recent_reviews", + :label => "To me", + :labelBackgroundColor => "pink", :recently_reviewed_commits => recently_reviewed_commits, :header => "My recently completed code review requests", :request_type => "Completed", @@ -512,7 +514,7 @@ def ensure_required_params(*required_params) commits_with_uncompleted_reviews = ReviewRequest.commits_with_uncompleted_reviews(current_user.id) recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) requests_from_me = ReviewRequest.requests_from_me(current_user.id) - default_list = "uncompleted_reviews,recent_reviews,unresolved_comments,recent_comments,requests_from_me,comments_from_me" + default_list = "uncompleted_reviews,unresolved_comments,recent_reviews,recent_comments,requests_from_me,comments_from_me" review_list_order = (current_user.review_list_order || default_list).split(",") erb :reviews, :locals => { :commits_with_unresolved_comments => commits_with_unresolved_comments, diff --git a/public/css/saved_search.scss b/public/css/saved_search.scss index 4719c42b..f3b02b0a 100644 --- a/public/css/saved_search.scss +++ b/public/css/saved_search.scss @@ -172,6 +172,11 @@ } #reviewLists { + .headerLabel { + padding-left: 2px; + padding-right: 2px; + margin-right: 4px; + } a.delete { padding-right: 5px; float: right; diff --git a/views/_commits_with_comments_list.erb b/views/_commits_with_comments_list.erb index 6487721e..c2dfddd3 100644 --- a/views/_commits_with_comments_list.erb +++ b/views/_commits_with_comments_list.erb @@ -2,6 +2,7 @@ This view snippet is used for a list of code review requests or a list of commits with unresolved comments. View arguments: - list_id: a string that identifies the list + - label: a label in the header, such as "For me" - header: a string that is displayed above the list - review_list: an array where each element is a ReviewListEntry %> @@ -9,7 +10,10 @@
    -
    <%= header %>
    +
    + <%= label %> + <%= header %> +
    <% if review_list.empty? %>
    None.
    diff --git a/views/_review_request_list.erb b/views/_review_request_list.erb index 9ece92d8..c5b7f533 100644 --- a/views/_review_request_list.erb +++ b/views/_review_request_list.erb @@ -2,6 +2,8 @@ This view snippet is used for a list of code review requests or a list of commits with unresolved comments. View arguments: - list_id: a string that identifies the list + - label: a label in the header, such as "For me" + - labelBackgroundColor: the background color for the label - header: a string that is displayed above the list - review_list: an array where each element is a ReviewListEntry - request_type: either "Requested" or "Completed" @@ -10,7 +12,10 @@
    -
    <%= header %>
    +
    + <%= label %> + <%= header %> +
    <% if review_list.empty? %>
    None.
    diff --git a/views/reviews.erb b/views/reviews.erb index 3899c8fc..2c656071 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -5,40 +5,52 @@ <% if list_id == "uncompleted_reviews" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, - :header => "For me: code review requests", + :label => "To me", + :labelBackgroundColor => "pink", + :header => "Code review requests", :request_type => "Requested", :review_list => commits_with_uncompleted_reviews } %> <% end %> <% if list_id == "recent_reviews" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, - :header => "My recently completed code review requests", + :label => "To me", + :labelBackgroundColor => "pink", + :header => "Recently completed requests", :request_type => "Completed", :review_list => recently_reviewed_commits } %> <% end %> <% if list_id == "unresolved_comments" %> <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, - :header => "For me: commits with unresolved comments", + :label => "To me", + :labelBackgroundColor => "pink", + :header => "Unresolved comments", :review_list => commits_with_unresolved_comments } %> <% end %> <% if list_id == "recent_comments" %> <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, - :header => "My commits with recently resolved comments", + :label => "To me", + :labelBackgroundColor => "pink", + :header => "Recently resolved comments", :review_list => recently_resolved_comments } %> <% end %> <% if list_id == "requests_from_me" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, - :header => "From me: code review requests", + :label => "From me", + :labelBackgroundColor => "paleturquoise", + :header => "Code review requests", :request_type => "Requested", :review_list => requests_from_me } %> <% end %> <% if list_id == "comments_from_me" %> <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, - :header => "From me: commits with unresolved comments", + :label => "From me", + :labelBackgroundColor => "paleturquoise", + :header => "Unresolved comments", :review_list => comments_from_me } %> <% end %> From b2d44100f2cd565d23026f9343bb92feb92f01b2 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 21 Jan 2013 18:36:38 -0800 Subject: [PATCH 41/71] Better looking labels. --- barkeep_server.rb | 2 +- public/css/saved_search.scss | 11 +++++++++-- views/_commits_with_comments_list.erb | 3 ++- views/_review_request_list.erb | 4 ++-- views/reviews.erb | 12 ++++++------ 5 files changed, 20 insertions(+), 12 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index bbc0cd27..bf05cc08 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -394,7 +394,7 @@ def ensure_required_params(*required_params) erb :_review_request_list, :layout => false, :locals => { :list_id => "recent_reviews", :label => "To me", - :labelBackgroundColor => "pink", + :labelClass => "toMe", :recently_reviewed_commits => recently_reviewed_commits, :header => "My recently completed code review requests", :request_type => "Completed", diff --git a/public/css/saved_search.scss b/public/css/saved_search.scss index f3b02b0a..626bd9ae 100644 --- a/public/css/saved_search.scss +++ b/public/css/saved_search.scss @@ -173,10 +173,17 @@ #reviewLists { .headerLabel { - padding-left: 2px; - padding-right: 2px; + padding: 1px 4px; + font-size: 12px; + line-height: 18px; + color: white; + text-shadow: -1px -1px 0px black; + font-weight: 400; + border-radius: 3px; margin-right: 4px; } + .toMe { background: $coreRed; } + .fromMe { background: darkgreen; } a.delete { padding-right: 5px; float: right; diff --git a/views/_commits_with_comments_list.erb b/views/_commits_with_comments_list.erb index c2dfddd3..7fde43ab 100644 --- a/views/_commits_with_comments_list.erb +++ b/views/_commits_with_comments_list.erb @@ -3,6 +3,7 @@ View arguments: - list_id: a string that identifies the list - label: a label in the header, such as "For me" + - labelClass: the class to use for the label (this determines the css style such as background color) - header: a string that is displayed above the list - review_list: an array where each element is a ReviewListEntry %> @@ -11,7 +12,7 @@
    - <%= label %> + <%= label %> <%= header %>
    diff --git a/views/_review_request_list.erb b/views/_review_request_list.erb index c5b7f533..c9ecaa9e 100644 --- a/views/_review_request_list.erb +++ b/views/_review_request_list.erb @@ -3,7 +3,7 @@ View arguments: - list_id: a string that identifies the list - label: a label in the header, such as "For me" - - labelBackgroundColor: the background color for the label + - labelClass: the class to use for the label (this determines the css style such as background color) - header: a string that is displayed above the list - review_list: an array where each element is a ReviewListEntry - request_type: either "Requested" or "Completed" @@ -13,7 +13,7 @@
    - <%= label %> + <%= label %> <%= header %>
    diff --git a/views/reviews.erb b/views/reviews.erb index 2c656071..643da28f 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -6,7 +6,7 @@ <% if list_id == "uncompleted_reviews" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, :label => "To me", - :labelBackgroundColor => "pink", + :labelClass => "toMe", :header => "Code review requests", :request_type => "Requested", :review_list => commits_with_uncompleted_reviews } %> @@ -15,7 +15,7 @@ <% if list_id == "recent_reviews" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, :label => "To me", - :labelBackgroundColor => "pink", + :labelClass => "toMe", :header => "Recently completed requests", :request_type => "Completed", :review_list => recently_reviewed_commits } %> @@ -24,7 +24,7 @@ <% if list_id == "unresolved_comments" %> <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, :label => "To me", - :labelBackgroundColor => "pink", + :labelClass => "toMe", :header => "Unresolved comments", :review_list => commits_with_unresolved_comments } %> <% end %> @@ -32,7 +32,7 @@ <% if list_id == "recent_comments" %> <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, :label => "To me", - :labelBackgroundColor => "pink", + :labelClass => "toMe", :header => "Recently resolved comments", :review_list => recently_resolved_comments } %> <% end %> @@ -40,7 +40,7 @@ <% if list_id == "requests_from_me" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, :label => "From me", - :labelBackgroundColor => "paleturquoise", + :labelClass => "fromMe", :header => "Code review requests", :request_type => "Requested", :review_list => requests_from_me } %> @@ -49,7 +49,7 @@ <% if list_id == "comments_from_me" %> <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, :label => "From me", - :labelBackgroundColor => "paleturquoise", + :labelClass => "fromMe", :header => "Unresolved comments", :review_list => comments_from_me } %> <% end %> From 10441e9a75b1ee9c978bd5dd74ac517458884662 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 21 Jan 2013 20:58:46 -0800 Subject: [PATCH 42/71] Add 'closed_at' field to comments. This is the final state of a comment. --- migrations/20130121205431_add_closed_at_to_comments.rb | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 migrations/20130121205431_add_closed_at_to_comments.rb diff --git a/migrations/20130121205431_add_closed_at_to_comments.rb b/migrations/20130121205431_add_closed_at_to_comments.rb new file mode 100644 index 00000000..8b07fe6f --- /dev/null +++ b/migrations/20130121205431_add_closed_at_to_comments.rb @@ -0,0 +1,9 @@ +require "bundler/setup" +require "pathological" +require "migrations/migration_helper.rb" + +Sequel.migration do + change do + add_column :comments, :closed_at, DateTime + end +end From 4b36ec1b49fa1fb120d73eecf6345aee85047e1f Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 21 Jan 2013 21:11:09 -0800 Subject: [PATCH 43/71] Rename comments.completed_at to resolved_at. --- barkeep_server.rb | 2 +- ...210423_rename_completed_at_to_resolved_at.rb | 17 +++++++++++++++++ models/comment.rb | 4 ++-- models/commit.rb | 8 ++++---- views/_comment.erb | 2 +- 5 files changed, 25 insertions(+), 8 deletions(-) create mode 100644 migrations/20130121210423_rename_completed_at_to_resolved_at.rb diff --git a/barkeep_server.rb b/barkeep_server.rb index bf05cc08..8694057c 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -649,7 +649,7 @@ def create_comment(repo_name, sha, filename, line_number_string, text) :line_number => line_number, :user => current_user, :text => text, - :completed_at => nil, + :resolved_at => nil, :has_been_emailed => current_user.demo?) # Don't email comments made by demo users. comment end diff --git a/migrations/20130121210423_rename_completed_at_to_resolved_at.rb b/migrations/20130121210423_rename_completed_at_to_resolved_at.rb new file mode 100644 index 00000000..42221137 --- /dev/null +++ b/migrations/20130121210423_rename_completed_at_to_resolved_at.rb @@ -0,0 +1,17 @@ +require "bundler/setup" +require "pathological" +require "migrations/migration_helper.rb" + +Sequel.migration do + up do + alter_table(:comments) do + rename_column :completed_at, :resolved_at + end + end + + down do + alter_table(:comments) do + rename_column :resolved_at, :completed_at + end + end +end diff --git a/models/comment.rb b/models/comment.rb index f2d99f14..83f332c9 100644 --- a/models/comment.rb +++ b/models/comment.rb @@ -33,10 +33,10 @@ def general_comment?() commit_file_id.nil? end def file_comment?() !commit_file_id.nil? end def mark_resolved - self.completed_at = Time.now + self.resolved_at = Time.now end def mark_unresolved - self.completed_at = nil + self.resolved_at = nil end end diff --git a/models/commit.rb b/models/commit.rb index 9b618d91..42602c96 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -89,7 +89,7 @@ def self.commits_with_unresolved_comments(email) join(:comments, :commit_id => :id). join(:authors, :id => :commits__author_id). join(:users, :id => :comments__user_id). - filter(:authors__email => email, :comments__completed_at => nil). + filter(:authors__email => email, :comments__resolved_at => nil). exclude(:users__email => email). group_by(:commits__id).all create_review_list_entries(commits) @@ -101,10 +101,10 @@ def self.commits_with_recently_resolved_comments(email) join(:authors, :id => :commits__author_id). join(:users, :id => :comments__user_id). filter(:authors__email => email). - exclude(:comments__completed_at => nil). + exclude(:comments__resolved_at => nil). exclude(:users__email => email). group_by(:commits__id). - reverse_order(:completed_at).limit(5).all + reverse_order(:resolved_at).limit(5).all create_review_list_entries(commits) end @@ -113,7 +113,7 @@ def self.commits_with_unresolved_comments_from_me(email) join(:comments, :commit_id => :id). join(:authors, :id => :commits__author_id). join(:users, :id => :comments__user_id). - filter(:users__email => email, :comments__completed_at => nil). + filter(:users__email => email, :comments__resolved_at => nil). exclude(:authors__email => email). group_by(:commits__id).all create_review_list_entries(commits) diff --git a/views/_comment.erb b/views/_comment.erb index 0380e4fd..d4071da4 100644 --- a/views/_comment.erb +++ b/views/_comment.erb @@ -11,7 +11,7 @@
    <% if current_user %> <% if comment.user_id != comment.commit.author.user_id %> - <% if comment.completed_at %> + <% if comment.resolved_at %> <% else %> From 4afdca00408ee484eeda2521d2c917a91d0d1cfa Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Tue, 22 Jan 2013 11:16:23 -0800 Subject: [PATCH 44/71] Implement UI for resolving and closing comments. --- barkeep_server.rb | 9 ++++++++- models/comment.rb | 8 +++++++- public/coffee/commit.coffee | 36 +++++++++++++++++++++++++----------- public/css/styles.scss | 27 +++++++++++++++++++++++++-- views/_comment.erb | 8 +++++--- 5 files changed, 70 insertions(+), 18 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index 8694057c..11eeb283 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -363,7 +363,14 @@ def ensure_required_params(*required_params) nil end - post "/unresolve_comment" do + post "/close_comment" do + comment = validate_comment(params[:comment_id], true) + comment.mark_closed + comment.save + nil + end + + post "/reopen_comment" do comment = validate_comment(params[:comment_id], true) comment.mark_unresolved comment.save diff --git a/models/comment.rb b/models/comment.rb index 83f332c9..74658feb 100644 --- a/models/comment.rb +++ b/models/comment.rb @@ -33,10 +33,16 @@ def general_comment?() commit_file_id.nil? end def file_comment?() !commit_file_id.nil? end def mark_resolved - self.resolved_at = Time.now + self.resolved_at = Time.now.utc + self.closed_at = nil + end + + def mark_closed + self.closed_at = Time.now.utc end def mark_unresolved self.resolved_at = nil + self.closed_at = nil end end diff --git a/public/coffee/commit.coffee b/public/coffee/commit.coffee index eda96fa8..52abfdce 100644 --- a/public/coffee/commit.coffee +++ b/public/coffee/commit.coffee @@ -23,8 +23,9 @@ window.Commit = $("#disapproveButton").live "click", (e) => @onDisapproveClicked e $(".delete").live "click", (e) => @onCommentDelete e $(".edit").live "click", (e) => @onCommentEdit e - $(".resolve").live "click", (e) => @onCommentResolved e - $(".unresolve").live "click", (e) => @onCommentUnresolved e + $(".resolveComment").live "click", (e) => @onCommentResolved e + $(".closeComment").live "click", (e) => @onCommentClosed e + $(".reopenComment").live "click", (e) => @onCommentReopened e $("#sideBySideButton").live "click", => @toggleSideBySide true $("#requestReviewButton").click (e) => @toggleReviewRequest() $("#hideCommentButton").live "click", (e) => @toggleComments() @@ -395,23 +396,36 @@ window.Commit = data: { comment_id: commentId }, success: => comment = $(".comment[commentId='#{commentId}']") - button = comment.find(".resolve") - $(button).html("Unresolve") - $(button).removeClass("resolve") - $(button).addClass("unresolve") + button = comment.find(".resolveComment") + $(button).html("Close") + $(button).removeClass("resolveComment") + $(button).addClass("closeComment") - onCommentUnresolved: (e) -> + onCommentClosed: (e) -> commentId = $(e.target).parents(".comment").attr("commentId") $.ajax type: "post", - url: "/unresolve_comment", + url: "/close_comment", data: { comment_id: commentId }, success: => comment = $(".comment[commentId='#{commentId}']") - button = comment.find(".unresolve") + button = comment.find(".closeComment") + $(button).html("Reopen") + $(button).removeClass("closeComment") + $(button).addClass("reopenComment") + + onCommentReopened: (e) -> + commentId = $(e.target).parents(".comment").attr("commentId") + $.ajax + type: "post", + url: "/reopen_comment", + data: { comment_id: commentId }, + success: => + comment = $(".comment[commentId='#{commentId}']") + button = comment.find(".reopenComment") $(button).html("Resolve") - $(button).removeClass("unresolve") - $(button).addClass("resolve") + $(button).removeClass("reopenComment") + $(button).addClass("resolveComment") # TODO(caleb): Add a Snippet for comment forms instead of contacting the server (there's no server logic # needed here). diff --git a/public/css/styles.scss b/public/css/styles.scss index 93d956e9..7a76318a 100644 --- a/public/css/styles.scss +++ b/public/css/styles.scss @@ -144,7 +144,7 @@ button.fancy { } /* Our standard buttons are flat grey buttons. */ -button.standard, .unresolve, input[type=button], input[type=submit] { +button.standard, .reopenComment, input[type=button], input[type=submit] { border-radius: 3px; padding: 7px 12px; text-shadow: 0px -1px 1px #EEEEEE; @@ -171,7 +171,7 @@ button.standard, .unresolve, input[type=button], input[type=submit] { * This style is a hybrid between the standard buttons (Reply, Edit, Delete) and the "Approve Commit" * button. */ -button.resolve { +button.resolveComment { $resolveColor: #42C050; border-radius: 3px; padding: 7px 12px; @@ -194,6 +194,29 @@ button.resolve { &:active { background-image: none; } } +button.closeComment { + $resolveColor: #4250C0; + border-radius: 3px; + padding: 7px 12px; + border: 1px solid darken($resolveColor, 20%); + border-bottom: 1px solid darken($resolveColor, 25%); + text-shadow: 0px -1px 1px darken($resolveColor, 30%); + font-weight: bold; + cursor: pointer; + color: white; + + @include linear-gradient(lighten($resolveColor, 3%) 0%, darken($resolveColor, 15%) 95%, + $fallback: darken($resolveColor, 18%)); + + &:hover { + border: 1px solid darken(#890128, 25%); + @include linear-gradient(lighten($resolveColor, 10%) 0%, darken($resolveColor, 15%) 95%, + $fallback: darken($resolveColor, 18%)); + } + + &:active { background-image: none; } +} + /* jQuery autocomplete styles. */ .ui-widget { /* This needs some width set, but it will fit itself to the size of the autocompleting input box. */ diff --git a/views/_comment.erb b/views/_comment.erb index d4071da4..0f7c0c10 100644 --- a/views/_comment.erb +++ b/views/_comment.erb @@ -11,10 +11,12 @@
    <% if current_user %> <% if comment.user_id != comment.commit.author.user_id %> - <% if comment.resolved_at %> - + <% if comment.closed_at %> + + <% elsif comment.resolved_at %> + <% else %> - + <% end %> <% end %> <% if comment.line_number %> From a040a032bfa1445063c10dc9df3923ba149ab743 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Tue, 22 Jan 2013 16:27:27 -0800 Subject: [PATCH 45/71] Add an 'Unresolve' button popup when hovering over the comment buttons that change state. --- barkeep_server.rb | 13 ++++++++++--- models/comment.rb | 10 +++++++--- public/coffee/commit.coffee | 21 +++++++++++++++++++-- public/css/styles.scss | 21 ++++++++++++++++++++- views/_comment.erb | 3 +++ 5 files changed, 59 insertions(+), 9 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index 11eeb283..dfe37421 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -358,21 +358,28 @@ def ensure_required_params(*required_params) post "/resolve_comment" do comment = validate_comment(params[:comment_id], true) - comment.mark_resolved + comment.resolve + comment.save + nil + end + + post "/unresolve_comment" do + comment = validate_comment(params[:comment_id], true) + comment.unresolve comment.save nil end post "/close_comment" do comment = validate_comment(params[:comment_id], true) - comment.mark_closed + comment.close comment.save nil end post "/reopen_comment" do comment = validate_comment(params[:comment_id], true) - comment.mark_unresolved + comment.reopen comment.save nil end diff --git a/models/comment.rb b/models/comment.rb index 74658feb..3129984e 100644 --- a/models/comment.rb +++ b/models/comment.rb @@ -32,16 +32,20 @@ def general_comment?() commit_file_id.nil? end # True if this comment pertains to a particular file. def file_comment?() !commit_file_id.nil? end - def mark_resolved + def resolve self.resolved_at = Time.now.utc self.closed_at = nil end - def mark_closed + def close self.closed_at = Time.now.utc end - def mark_unresolved + def reopen + self.closed_at = nil + end + + def unresolve self.resolved_at = nil self.closed_at = nil end diff --git a/public/coffee/commit.coffee b/public/coffee/commit.coffee index 52abfdce..4eae35d8 100644 --- a/public/coffee/commit.coffee +++ b/public/coffee/commit.coffee @@ -24,6 +24,7 @@ window.Commit = $(".delete").live "click", (e) => @onCommentDelete e $(".edit").live "click", (e) => @onCommentEdit e $(".resolveComment").live "click", (e) => @onCommentResolved e + $(".unresolveComment").live "click", (e) => @onCommentUnresolved e $(".closeComment").live "click", (e) => @onCommentClosed e $(".reopenComment").live "click", (e) => @onCommentReopened e $("#sideBySideButton").live "click", => @toggleSideBySide true @@ -401,6 +402,22 @@ window.Commit = $(button).removeClass("resolveComment") $(button).addClass("closeComment") + onCommentUnresolved: (e) -> + commentId = $(e.target).parents(".comment").attr("commentId") + $.ajax + type: "post", + url: "/unresolve_comment", + data: { comment_id: commentId }, + success: => + comment = $(".comment[commentId='#{commentId}']") + button = comment.find(".buttonDropDown button:first") + $(button).html("Resolve") + if button.hasClass("closeComment") + $(button).removeClass("closeComment") + else if button.hasClass("reopenComment") + $(button).removeClass("reopenComment") + $(button).addClass("resolveComment") + onCommentClosed: (e) -> commentId = $(e.target).parents(".comment").attr("commentId") $.ajax @@ -423,9 +440,9 @@ window.Commit = success: => comment = $(".comment[commentId='#{commentId}']") button = comment.find(".reopenComment") - $(button).html("Resolve") + $(button).html("Close") $(button).removeClass("reopenComment") - $(button).addClass("resolveComment") + $(button).addClass("closeComment") # TODO(caleb): Add a Snippet for comment forms instead of contacting the server (there's no server logic # needed here). diff --git a/public/css/styles.scss b/public/css/styles.scss index 7a76318a..5ac13e41 100644 --- a/public/css/styles.scss +++ b/public/css/styles.scss @@ -144,7 +144,7 @@ button.fancy { } /* Our standard buttons are flat grey buttons. */ -button.standard, .reopenComment, input[type=button], input[type=submit] { +button.standard, .reopenComment, .commentButton2, input[type=button], input[type=submit] { border-radius: 3px; padding: 7px 12px; text-shadow: 0px -1px 1px #EEEEEE; @@ -173,6 +173,7 @@ button.standard, .reopenComment, input[type=button], input[type=submit] { */ button.resolveComment { $resolveColor: #42C050; + width: 84px; border-radius: 3px; padding: 7px 12px; border: 1px solid darken($resolveColor, 20%); @@ -196,6 +197,7 @@ button.resolveComment { button.closeComment { $resolveColor: #4250C0; + width: 84px; border-radius: 3px; padding: 7px 12px; border: 1px solid darken($resolveColor, 20%); @@ -217,6 +219,23 @@ button.closeComment { &:active { background-image: none; } } +button.reopenComment { + width: 84px; +} + +.buttonDropDown { + position: relative; + &:hover .unresolveComment { visibility: visible; } +} + +.unresolveComment { + position: absolute; + width: 84px; + top: 23px; + left: 0px; + visibility: hidden; +} + /* jQuery autocomplete styles. */ .ui-widget { /* This needs some width set, but it will fit itself to the size of the autocompleting input box. */ diff --git a/views/_comment.erb b/views/_comment.erb index 0f7c0c10..aeb8b16e 100644 --- a/views/_comment.erb +++ b/views/_comment.erb @@ -11,6 +11,7 @@
    <% if current_user %> <% if comment.user_id != comment.commit.author.user_id %> + <% if comment.closed_at %> <% elsif comment.resolved_at %> @@ -18,6 +19,8 @@ <% else %> <% end %> + + <% end %> <% if comment.line_number %> From f8158a48cb8fce6ed0517db8ddf9c49af8b084c3 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Tue, 22 Jan 2013 16:39:09 -0800 Subject: [PATCH 46/71] Add 'action_required' to 'comments' table. --- .../20130122162829_add_needs_action_to_comments.rb | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 migrations/20130122162829_add_needs_action_to_comments.rb diff --git a/migrations/20130122162829_add_needs_action_to_comments.rb b/migrations/20130122162829_add_needs_action_to_comments.rb new file mode 100644 index 00000000..b2c1121f --- /dev/null +++ b/migrations/20130122162829_add_needs_action_to_comments.rb @@ -0,0 +1,9 @@ +require "bundler/setup" +require "pathological" +require "migrations/migration_helper.rb" + +Sequel.migration do + change do + add_column :comments, :action_required, TrueClass + end +end From b2653d4e0d0c4ef350a609db8dc7cd7a45329208 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Tue, 22 Jan 2013 22:01:15 -0800 Subject: [PATCH 47/71] Add 'Response required' checkbox to comments. --- barkeep_server.rb | 21 +++++++++++++-------- public/coffee/commit.coffee | 28 ++++++++++++++++++---------- public/coffee/snippets.coffee | 9 +++++++-- public/css/diff_view.scss | 6 ++++++ views/_comment.erb | 7 +++++-- views/_comment_form.erb | 6 +++++- views/commit.erb | 3 ++- 7 files changed, 56 insertions(+), 24 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index dfe37421..3773878d 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -323,7 +323,8 @@ def ensure_required_params(*required_params) :repo_name => params[:repo_name], :sha => params[:sha], :filename => params[:filename], - :line_number => params[:line_number] + :line_number => params[:line_number], + :action_required => params[:action_required] == "true", } end @@ -339,13 +340,16 @@ def ensure_required_params(*required_params) if params[:comment_id] comment = validate_comment(params[:comment_id]) comment.text = params[:text] + comment.action_required = params[:action_required] == "true" comment.save - next comment.filter_text - end - begin - comment = create_comment(*[:repo_name, :sha, :filename, :line_number, :text].map { |f| params[f] }) - rescue RuntimeError => e - halt 400, e.message + else + begin + values = [:repo_name, :sha, :filename, :line_number, :text].map { |f| params[f] } + values << (params[:action_required] == "true") + comment = create_comment(*values) + rescue RuntimeError => e + halt 400, e.message + end end erb :_comment, :layout => false, :locals => { :comment => comment } end @@ -648,7 +652,7 @@ def link_author_to_user(email) end end - def create_comment(repo_name, sha, filename, line_number_string, text) + def create_comment(repo_name, sha, filename, line_number_string, text, action_required) commit = MetaRepo.instance.db_commit(repo_name, sha) raise "No such commit." unless commit file = nil @@ -664,6 +668,7 @@ def create_comment(repo_name, sha, filename, line_number_string, text) :user => current_user, :text => text, :resolved_at => nil, + :action_required => action_required, :has_been_emailed => current_user.demo?) # Don't email comments made by demo users. comment end diff --git a/public/coffee/commit.coffee b/public/coffee/commit.coffee index 4eae35d8..70f921bf 100644 --- a/public/coffee/commit.coffee +++ b/public/coffee/commit.coffee @@ -11,10 +11,10 @@ window.Commit = SIDE_BY_SIDE_CODE_WIDTH: 830 init: -> - $(".addCommentButton").click (e) => @onAddCommentMouseAction e + $(".addCommentButton").click (e) => @onAddCommentMouseAction(e, true) $("a.tipsyCommentCount").tipsy(gravity: "w") - $(".diffLine").dblclick (e) => @onAddCommentMouseAction e - $(".reply").live "click", (e) => @onAddCommentMouseAction e + $(".diffLine").dblclick (e) => @onAddCommentMouseAction(e, true) + $(".reply").live "click", (e) => @onAddCommentMouseAction(e, false) $(".diffLine").hover(((e) => @selectLine(e)), ((e) => @clearSelectedLine())) $(".commentForm").live "submit", (e) => @onCommentSubmit e $(".commentPreview").click (e) => @onCommentPreview e @@ -330,7 +330,7 @@ window.Commit = refreshLine?.hide() refreshLine?.show(1) - onAddCommentMouseAction: (e) -> + onAddCommentMouseAction: (e, action_required) -> $target = $(e.target) unless ($target.parents(".commentBody").size() > 0) || e.target.tagName.toLowerCase() in ["input", "textarea"] @@ -350,13 +350,14 @@ window.Commit = filename = codeLines.parents(".file").attr("filename") sha = codeLines.parents("#commit").attr("sha") repoName = codeLines.parents("#commit").attr("repo") - @createCommentForm(codeLines, repoName, sha, filename, lineNumber) + @createCommentForm(codeLines, repoName, sha, filename, lineNumber, action_required) onCommentEdit: (e) -> # Use the comment ID instead of generating form ID since left and right tables have the same comments. comment = $(".comment[commentId='#{$(e.target).parents(".comment").attr("commentId")}']") if comment.find(".commentEditForm").size() > 0 then return - commentEdit = $(Snippets.commentForm(true, true)) + actionRequired = comment.data("actionRequired") + commentEdit = $(Snippets.commentForm(true, true, actionRequired)) commentEdit.find(".commentText").html($(e.target).parents(".comment").data("commentRaw")) commentEdit.find(".commentCancel").click @onCommentEditCancel comment.append(commentEdit).find(".commentBody").hide() @@ -375,20 +376,25 @@ window.Commit = target = $(e.currentTarget) text = target.find(".commentText").val() if text == "" then return - commentId = target.parents(".comment").attr("commentId") + comment = target.parents(".comment") + commentId = comment.attr("commentId") + actionRequired = comment.find(".actionRequired").is(":checked") $.ajax type: "post", url: e.currentTarget.action, data: { comment_id: commentId text: text + action_required: actionRequired }, success: (html) -> comment = $(".comment[commentId='#{commentId}']") - comment.data("commentRaw", text) - comment.find(".commentBody").html(html) + container = comment.parents(".commentAndAnchorContainer") + container.before(html) + container.remove() comment.find(".commentCancel").click() + onCommentResolved: (e) -> commentId = $(e.target).parents(".comment").attr("commentId") $.ajax @@ -446,7 +452,7 @@ window.Commit = # TODO(caleb): Add a Snippet for comment forms instead of contacting the server (there's no server logic # needed here). - createCommentForm: (codeLine, repoName, sha, filename, lineNumber) -> + createCommentForm: (codeLine, repoName, sha, filename, lineNumber, action_required) -> $.ajax type: "get" url: "/comment_form" @@ -455,6 +461,7 @@ window.Commit = sha: sha filename: filename line_number: lineNumber + action_required: action_required success: (html) => comment = $(html) commentForm = comment.find(".commentForm") @@ -500,6 +507,7 @@ window.Commit = form = if file.size() > 0 then file.find(".commentForm[form-id='" + formId + "']") else target data = {} target.find("input, textarea").each (i,e) -> data[e.name] = e.value if e.name + data["action_required"] = form.find(".actionRequired").is(":checked") $.ajax type: "POST", data: data, diff --git a/public/coffee/snippets.coffee b/public/coffee/snippets.coffee index 1db19c4b..09ea6f96 100644 --- a/public/coffee/snippets.coffee +++ b/public/coffee/snippets.coffee @@ -3,7 +3,7 @@ window.Snippets = # Comment form html. This handles adding and editing comments - commentForm: (inline, edit, hiddenFields) -> + commentForm: (inline, edit, action_required, hiddenFields) -> className = if edit then "commentEditForm" else "commentForm" submitValue = if edit then "Save Edit" else "Post Comment" header = if edit then "" else """ @@ -19,7 +19,12 @@ window.Snippets =
    - #{if inline then ""} + #{if inline then "" else ""} +
    + + +
    " """ diff --git a/public/css/diff_view.scss b/public/css/diff_view.scss index 5997838d..dc35505b 100644 --- a/public/css/diff_view.scss +++ b/public/css/diff_view.scss @@ -388,6 +388,12 @@ @include box-flex(1); @include box-ordinal-group(1); margin-left: 10px; + + label { + font-size: 14px; + margin-left: 20px; + input[type=checkbox] { margin-right: 5px; } + } } .commentCancel { @include display-box; diff --git a/views/_comment.erb b/views/_comment.erb index aeb8b16e..8df4cedc 100644 --- a/views/_comment.erb +++ b/views/_comment.erb @@ -1,8 +1,10 @@ <% if comment %> +
    + data-comment-raw="<%= CGI::escape_html(comment.text)%>" + data-action-required="<%= comment.action_required %>">
    <%= CGI::escape_html(comment.user.name) %> @@ -10,7 +12,7 @@ <%= comment.created_at.to_pretty %>
    <% if current_user %> - <% if comment.user_id != comment.commit.author.user_id %> + <% if comment.action_required %> <% if comment.closed_at %> @@ -35,4 +37,5 @@
    <%= comment.filter_text %>
    +
    <% end %> diff --git a/views/_comment_form.erb b/views/_comment_form.erb index 298496f4..f91bed1b 100644 --- a/views/_comment_form.erb +++ b/views/_comment_form.erb @@ -13,7 +13,11 @@ <% if line_number %> <% end %> -
    +
    + + +
    diff --git a/views/commit.erb b/views/commit.erb index e78a054e..9ad24c7d 100644 --- a/views/commit.erb +++ b/views/commit.erb @@ -107,7 +107,8 @@ :repo_name => commit.git_repo.name, :sha => commit.sha, :filename => nil, - :line_number => nil + :line_number => nil, + :action_required => current_user.author.nil? || current_user.author.id != commit.author.id, } %>
    <% else %> From 45cad0d2119f6b50a7c550311cea77843d187225 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Wed, 23 Jan 2013 13:00:51 -0800 Subject: [PATCH 48/71] Make the 'Preview Comment' button work when editing a comment. --- public/coffee/commit.coffee | 3 ++- public/coffee/snippets.coffee | 1 + public/css/diff_view.scss | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/public/coffee/commit.coffee b/public/coffee/commit.coffee index 70f921bf..5b3c527f 100644 --- a/public/coffee/commit.coffee +++ b/public/coffee/commit.coffee @@ -360,6 +360,7 @@ window.Commit = commentEdit = $(Snippets.commentForm(true, true, actionRequired)) commentEdit.find(".commentText").html($(e.target).parents(".comment").data("commentRaw")) commentEdit.find(".commentCancel").click @onCommentEditCancel + commentEdit.find(".commentPreview").click @onCommentPreview comment.append(commentEdit).find(".commentBody").hide() textarea = comment.find(".commentText") KeyboardShortcuts.createShortcutContext textarea @@ -552,7 +553,7 @@ window.Commit = # Toggle preview/editing mode onCommentPreview: (e) -> e.stopPropagation() - comment = $(e.target).parents(".commentForm") + comment = $(e.target).parents(".commentForm,.commentEditForm") preview = comment.find(".commentPreviewText") textarea = comment.find(".commentText") previewButton = comment.find(".commentPreview") diff --git a/public/coffee/snippets.coffee b/public/coffee/snippets.coffee index 09ea6f96..7c6c563c 100644 --- a/public/coffee/snippets.coffee +++ b/public/coffee/snippets.coffee @@ -17,6 +17,7 @@ window.Snippets =
    #{header} +
    #{if inline then "" else ""} diff --git a/public/css/diff_view.scss b/public/css/diff_view.scss index dc35505b..29675d0e 100644 --- a/public/css/diff_view.scss +++ b/public/css/diff_view.scss @@ -304,6 +304,7 @@ } .commentPreviewText { + font: 14px $sansFont; border-bottom: $standardBorder; display: none; } From d2e6941ba352bc35ba2d436a329d7dda68d8f26a Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Wed, 23 Jan 2013 18:15:13 -0800 Subject: [PATCH 49/71] Change the color of a comment header to reflect its state (New, Resolved, Closed). --- public/css/diff_view.scss | 13 ++++++++++ public/css/styles.scss | 52 ++------------------------------------- views/_comment.erb | 12 ++++++++- 3 files changed, 26 insertions(+), 51 deletions(-) diff --git a/public/css/diff_view.scss b/public/css/diff_view.scss index 29675d0e..858ccc8b 100644 --- a/public/css/diff_view.scss +++ b/public/css/diff_view.scss @@ -409,6 +409,19 @@ } } +.comment div.heading { + &.newComment { + $color: #FFDDDD; + @include linear-gradient(lighten($color, 3%) 0%, darken($color, 15%) 95%, + $fallback: darken($color, 18%)); + } + &.resolvedComment { + $color: #E0FFFF; + @include linear-gradient(lighten($color, 3%) 0%, darken($color, 15%) 95%, + $fallback: darken($color, 18%)); + } +} + // Comment edit form is not a panel, so doesn't need panel CSS .commentEditForm { margin: 0; diff --git a/public/css/styles.scss b/public/css/styles.scss index 5ac13e41..1b97fbb1 100644 --- a/public/css/styles.scss +++ b/public/css/styles.scss @@ -144,7 +144,7 @@ button.fancy { } /* Our standard buttons are flat grey buttons. */ -button.standard, .reopenComment, .commentButton2, input[type=button], input[type=submit] { +button.standard, .reopenComment, .resolveComment, .closeComment, input[type=button], input[type=submit] { border-radius: 3px; padding: 7px 12px; text-shadow: 0px -1px 1px #EEEEEE; @@ -171,55 +171,7 @@ button.standard, .reopenComment, .commentButton2, input[type=button], input[type * This style is a hybrid between the standard buttons (Reply, Edit, Delete) and the "Approve Commit" * button. */ -button.resolveComment { - $resolveColor: #42C050; - width: 84px; - border-radius: 3px; - padding: 7px 12px; - border: 1px solid darken($resolveColor, 20%); - border-bottom: 1px solid darken($resolveColor, 25%); - text-shadow: 0px -1px 1px darken($resolveColor, 30%); - font-weight: bold; - cursor: pointer; - color: white; - - @include linear-gradient(lighten($resolveColor, 3%) 0%, darken($resolveColor, 15%) 95%, - $fallback: darken($resolveColor, 18%)); - - &:hover { - border: 1px solid darken(#890128, 25%); - @include linear-gradient(lighten($resolveColor, 10%) 0%, darken($resolveColor, 15%) 95%, - $fallback: darken($resolveColor, 18%)); - } - - &:active { background-image: none; } -} - -button.closeComment { - $resolveColor: #4250C0; - width: 84px; - border-radius: 3px; - padding: 7px 12px; - border: 1px solid darken($resolveColor, 20%); - border-bottom: 1px solid darken($resolveColor, 25%); - text-shadow: 0px -1px 1px darken($resolveColor, 30%); - font-weight: bold; - cursor: pointer; - color: white; - - @include linear-gradient(lighten($resolveColor, 3%) 0%, darken($resolveColor, 15%) 95%, - $fallback: darken($resolveColor, 18%)); - - &:hover { - border: 1px solid darken(#890128, 25%); - @include linear-gradient(lighten($resolveColor, 10%) 0%, darken($resolveColor, 15%) 95%, - $fallback: darken($resolveColor, 18%)); - } - - &:active { background-image: none; } -} - -button.reopenComment { +button.resolveComment, button.closeComment, button.reopenComment { width: 84px; } diff --git a/views/_comment.erb b/views/_comment.erb index 8df4cedc..2c122f79 100644 --- a/views/_comment.erb +++ b/views/_comment.erb @@ -1,11 +1,21 @@ <% if comment %> + <% state = "" %> + <% if comment.action_required %> + <% if comment.resolved_at.nil? %> + <% state = "newComment" %> + <% elsif comment.closed_at.nil? %> + <% state = "resolvedComment" %> + <% else %> + <% state = "closedComment" %> + <% end %> + <% end %>
    -
    +
    <%= CGI::escape_html(comment.user.name) %> commented From 527159080458875a9a56055b9ca967d6826f31ea Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Wed, 23 Jan 2013 18:16:17 -0800 Subject: [PATCH 50/71] Change wording from 'Response required' to more polite 'Response requested'. --- views/_comment_form.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/views/_comment_form.erb b/views/_comment_form.erb index f91bed1b..2e1c9c32 100644 --- a/views/_comment_form.erb +++ b/views/_comment_form.erb @@ -16,7 +16,7 @@
    + <%= "checked" if action_required %> />Response requested
    From dd84c14af4a5343bdf26fa9c5e17be86b9f30a10 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Wed, 23 Jan 2013 18:18:29 -0800 Subject: [PATCH 51/71] Show 'actionable comments' (to-be-resolved and to-be-closed) in Reviews. --- barkeep_server.rb | 16 ++++++---------- models/commit.rb | 17 +++++++++++++++++ views/reviews.erb | 36 ++++++++++-------------------------- 3 files changed, 33 insertions(+), 36 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index 3773878d..8a464306 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -411,7 +411,7 @@ def ensure_required_params(*required_params) recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) erb :_review_request_list, :layout => false, :locals => { :list_id => "recent_reviews", - :label => "To me", + :label => "For me", :labelClass => "toMe", :recently_reviewed_commits => recently_reviewed_commits, :header => "My recently completed code review requests", @@ -526,21 +526,17 @@ def ensure_required_params(*required_params) end get "/reviews" do - commits_with_unresolved_comments = Commit.commits_with_unresolved_comments(current_user.email) - recently_resolved_comments = Commit.commits_with_recently_resolved_comments(current_user.email) - comments_from_me = Commit.commits_with_unresolved_comments_from_me(current_user.email) - commits_with_uncompleted_reviews = ReviewRequest.commits_with_uncompleted_reviews(current_user.id) + uncompleted_reviews = ReviewRequest.commits_with_uncompleted_reviews(current_user.id) + actionable_comments = Commit.commits_with_actionable_comments_for_user(current_user.id) recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) requests_from_me = ReviewRequest.requests_from_me(current_user.id) - default_list = "uncompleted_reviews,unresolved_comments,recent_reviews,recent_comments,requests_from_me,comments_from_me" + default_list = "uncompleted_reviews,actionable_comments,recent_reviews,recent_comments,requests_from_me,comments_from_me" review_list_order = (current_user.review_list_order || default_list).split(",") erb :reviews, :locals => { - :commits_with_unresolved_comments => commits_with_unresolved_comments, - :commits_with_uncompleted_reviews => commits_with_uncompleted_reviews, - :recently_resolved_comments => recently_resolved_comments, + :commits_with_uncompleted_reviews => uncompleted_reviews, + :commits_with_actionable_comments => actionable_comments, :recently_reviewed_commits => recently_reviewed_commits, :requests_from_me => requests_from_me, - :comments_from_me => comments_from_me, :review_list_ids => review_list_order, } end diff --git a/models/commit.rb b/models/commit.rb index 42602c96..1359df84 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -118,4 +118,21 @@ def self.commits_with_unresolved_comments_from_me(email) group_by(:commits__id).all create_review_list_entries(commits) end + + # Selects for the given user all the commits with "actionable" comments, that is, comments that + # match the following conditions: + # 1. Comments that have "action_required" and are not closed, and + # 2a. Comments that were made on one of this user's commits (including any comments by this user), + # and are not resolved (and not closed), or + # 2b. Comments that this user made on some commit that are resolved (but not closed). + def self.commits_with_actionable_comments_for_user(user_id) + commits = Commit. + join(:comments, :commit_id => :id). + join(:authors, :id => :commits__author_id). + filter(:action_required => true, :comments__closed_at => nil). + where({ :authors__user_id => user_id, :comments__resolved_at => nil } | + { :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }). + group_by(:commits__id).all + create_review_list_entries(commits) + end end diff --git a/views/reviews.erb b/views/reviews.erb index 643da28f..051e83b1 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -5,38 +5,30 @@ <% if list_id == "uncompleted_reviews" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, - :label => "To me", + :label => "For me", :labelClass => "toMe", :header => "Code review requests", :request_type => "Requested", :review_list => commits_with_uncompleted_reviews } %> <% end %> +<% if list_id == "actionable_comments" %> + <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, + :label => "For me", + :labelClass => "toMe", + :header => "Actionable comments", + :review_list => commits_with_actionable_comments } %> +<% end %> + <% if list_id == "recent_reviews" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, - :label => "To me", + :label => "For me", :labelClass => "toMe", :header => "Recently completed requests", :request_type => "Completed", :review_list => recently_reviewed_commits } %> <% end %> -<% if list_id == "unresolved_comments" %> - <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, - :label => "To me", - :labelClass => "toMe", - :header => "Unresolved comments", - :review_list => commits_with_unresolved_comments } %> -<% end %> - -<% if list_id == "recent_comments" %> - <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, - :label => "To me", - :labelClass => "toMe", - :header => "Recently resolved comments", - :review_list => recently_resolved_comments } %> -<% end %> - <% if list_id == "requests_from_me" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, :label => "From me", @@ -46,13 +38,5 @@ :review_list => requests_from_me } %> <% end %> -<% if list_id == "comments_from_me" %> - <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, - :label => "From me", - :labelClass => "fromMe", - :header => "Unresolved comments", - :review_list => comments_from_me } %> -<% end %> - <% end %>
    From 0317068af9fcf15572f5308e52731beaf4879d06 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Wed, 23 Jan 2013 18:40:43 -0800 Subject: [PATCH 52/71] Update the comment from the server when changing state (resolving, closing, ...) to get correct styling. --- barkeep_server.rb | 8 ++++---- public/coffee/commit.coffee | 39 +++++++++++++++---------------------- 2 files changed, 20 insertions(+), 27 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index 8a464306..354416c7 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -364,28 +364,28 @@ def ensure_required_params(*required_params) comment = validate_comment(params[:comment_id], true) comment.resolve comment.save - nil + erb :_comment, :layout => false, :locals => { :comment => comment } end post "/unresolve_comment" do comment = validate_comment(params[:comment_id], true) comment.unresolve comment.save - nil + erb :_comment, :layout => false, :locals => { :comment => comment } end post "/close_comment" do comment = validate_comment(params[:comment_id], true) comment.close comment.save - nil + erb :_comment, :layout => false, :locals => { :comment => comment } end post "/reopen_comment" do comment = validate_comment(params[:comment_id], true) comment.reopen comment.save - nil + erb :_comment, :layout => false, :locals => { :comment => comment } end post "/approve_commit" do diff --git a/public/coffee/commit.coffee b/public/coffee/commit.coffee index 5b3c527f..0037ae1c 100644 --- a/public/coffee/commit.coffee +++ b/public/coffee/commit.coffee @@ -402,12 +402,11 @@ window.Commit = type: "post", url: "/resolve_comment", data: { comment_id: commentId }, - success: => + success: (html) => comment = $(".comment[commentId='#{commentId}']") - button = comment.find(".resolveComment") - $(button).html("Close") - $(button).removeClass("resolveComment") - $(button).addClass("closeComment") + container = comment.parents(".commentAndAnchorContainer") + container.before(html) + container.remove() onCommentUnresolved: (e) -> commentId = $(e.target).parents(".comment").attr("commentId") @@ -415,15 +414,11 @@ window.Commit = type: "post", url: "/unresolve_comment", data: { comment_id: commentId }, - success: => + success: (html) => comment = $(".comment[commentId='#{commentId}']") - button = comment.find(".buttonDropDown button:first") - $(button).html("Resolve") - if button.hasClass("closeComment") - $(button).removeClass("closeComment") - else if button.hasClass("reopenComment") - $(button).removeClass("reopenComment") - $(button).addClass("resolveComment") + container = comment.parents(".commentAndAnchorContainer") + container.before(html) + container.remove() onCommentClosed: (e) -> commentId = $(e.target).parents(".comment").attr("commentId") @@ -431,12 +426,11 @@ window.Commit = type: "post", url: "/close_comment", data: { comment_id: commentId }, - success: => + success: (html) => comment = $(".comment[commentId='#{commentId}']") - button = comment.find(".closeComment") - $(button).html("Reopen") - $(button).removeClass("closeComment") - $(button).addClass("reopenComment") + container = comment.parents(".commentAndAnchorContainer") + container.before(html) + container.remove() onCommentReopened: (e) -> commentId = $(e.target).parents(".comment").attr("commentId") @@ -444,12 +438,11 @@ window.Commit = type: "post", url: "/reopen_comment", data: { comment_id: commentId }, - success: => + success: (html) => comment = $(".comment[commentId='#{commentId}']") - button = comment.find(".reopenComment") - $(button).html("Close") - $(button).removeClass("reopenComment") - $(button).addClass("closeComment") + container = comment.parents(".commentAndAnchorContainer") + container.before(html) + container.remove() # TODO(caleb): Add a Snippet for comment forms instead of contacting the server (there's no server logic # needed here). From 4ade79966dc183c28137ae2379f949d3ed2f5a14 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Wed, 23 Jan 2013 20:00:38 -0800 Subject: [PATCH 53/71] Style changes to reviews.erb for the parameter lists. No logic changes. --- views/reviews.erb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/views/reviews.erb b/views/reviews.erb index 051e83b1..5040ed7b 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -4,7 +4,8 @@ <% review_list_ids.each do |list_id| %> <% if list_id == "uncompleted_reviews" %> - <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, + <%= erb :_review_request_list, :layout => false, :locals => { + :list_id => list_id, :label => "For me", :labelClass => "toMe", :header => "Code review requests", @@ -13,7 +14,8 @@ <% end %> <% if list_id == "actionable_comments" %> - <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, + <%= erb :_commits_with_comments_list, :layout => false, :locals => { + :list_id => list_id, :label => "For me", :labelClass => "toMe", :header => "Actionable comments", @@ -21,7 +23,8 @@ <% end %> <% if list_id == "recent_reviews" %> - <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, + <%= erb :_review_request_list, :layout => false, :locals => { + :list_id => list_id, :label => "For me", :labelClass => "toMe", :header => "Recently completed requests", @@ -30,7 +33,8 @@ <% end %> <% if list_id == "requests_from_me" %> - <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, + <%= erb :_review_request_list, :layout => false, :locals => { + :list_id => list_id, :label => "From me", :labelClass => "fromMe", :header => "Code review requests", From 59069702b460c0e20174edec20bac8cacbf4a273 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Wed, 23 Jan 2013 20:35:03 -0800 Subject: [PATCH 54/71] Back to 2 visible comment states in the UI. --- barkeep_server.rb | 7 ------- public/coffee/commit.coffee | 26 -------------------------- public/css/diff_view.scss | 15 ++++----------- public/css/styles.scss | 24 +----------------------- views/_comment.erb | 13 +++---------- 5 files changed, 8 insertions(+), 77 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index 354416c7..b6bd1749 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -381,13 +381,6 @@ def ensure_required_params(*required_params) erb :_comment, :layout => false, :locals => { :comment => comment } end - post "/reopen_comment" do - comment = validate_comment(params[:comment_id], true) - comment.reopen - comment.save - erb :_comment, :layout => false, :locals => { :comment => comment } - end - post "/approve_commit" do commit = MetaRepo.instance.db_commit(params[:repo_name], params[:commit_sha]) halt 400 unless commit diff --git a/public/coffee/commit.coffee b/public/coffee/commit.coffee index 0037ae1c..03175dd0 100644 --- a/public/coffee/commit.coffee +++ b/public/coffee/commit.coffee @@ -25,8 +25,6 @@ window.Commit = $(".edit").live "click", (e) => @onCommentEdit e $(".resolveComment").live "click", (e) => @onCommentResolved e $(".unresolveComment").live "click", (e) => @onCommentUnresolved e - $(".closeComment").live "click", (e) => @onCommentClosed e - $(".reopenComment").live "click", (e) => @onCommentReopened e $("#sideBySideButton").live "click", => @toggleSideBySide true $("#requestReviewButton").click (e) => @toggleReviewRequest() $("#hideCommentButton").live "click", (e) => @toggleComments() @@ -420,30 +418,6 @@ window.Commit = container.before(html) container.remove() - onCommentClosed: (e) -> - commentId = $(e.target).parents(".comment").attr("commentId") - $.ajax - type: "post", - url: "/close_comment", - data: { comment_id: commentId }, - success: (html) => - comment = $(".comment[commentId='#{commentId}']") - container = comment.parents(".commentAndAnchorContainer") - container.before(html) - container.remove() - - onCommentReopened: (e) -> - commentId = $(e.target).parents(".comment").attr("commentId") - $.ajax - type: "post", - url: "/reopen_comment", - data: { comment_id: commentId }, - success: (html) => - comment = $(".comment[commentId='#{commentId}']") - container = comment.parents(".commentAndAnchorContainer") - container.before(html) - container.remove() - # TODO(caleb): Add a Snippet for comment forms instead of contacting the server (there's no server logic # needed here). createCommentForm: (codeLine, repoName, sha, filename, lineNumber, action_required) -> diff --git a/public/css/diff_view.scss b/public/css/diff_view.scss index 858ccc8b..88367049 100644 --- a/public/css/diff_view.scss +++ b/public/css/diff_view.scss @@ -409,17 +409,10 @@ } } -.comment div.heading { - &.newComment { - $color: #FFDDDD; - @include linear-gradient(lighten($color, 3%) 0%, darken($color, 15%) 95%, - $fallback: darken($color, 18%)); - } - &.resolvedComment { - $color: #E0FFFF; - @include linear-gradient(lighten($color, 3%) 0%, darken($color, 15%) 95%, - $fallback: darken($color, 18%)); - } +.comment div.heading.newComment { + $color: #FFDDDD; + @include linear-gradient(lighten($color, 3%) 0%, darken($color, 15%) 95%, + $fallback: darken($color, 18%)); } // Comment edit form is not a panel, so doesn't need panel CSS diff --git a/public/css/styles.scss b/public/css/styles.scss index 1b97fbb1..d265cb9c 100644 --- a/public/css/styles.scss +++ b/public/css/styles.scss @@ -144,7 +144,7 @@ button.fancy { } /* Our standard buttons are flat grey buttons. */ -button.standard, .reopenComment, .resolveComment, .closeComment, input[type=button], input[type=submit] { +button.standard, .resolveComment, .unresolveComment, input[type=button], input[type=submit] { border-radius: 3px; padding: 7px 12px; text-shadow: 0px -1px 1px #EEEEEE; @@ -166,28 +166,6 @@ button.standard, .reopenComment, .resolveComment, .closeComment, input[type=butt .webkit button.standard { font-weight: bold; } -/* The "Resolve" button needs to stand out a bit from the other buttons because it indicates - * the state of the comment and users will want to find all their unresolved comments. - * This style is a hybrid between the standard buttons (Reply, Edit, Delete) and the "Approve Commit" - * button. - */ -button.resolveComment, button.closeComment, button.reopenComment { - width: 84px; -} - -.buttonDropDown { - position: relative; - &:hover .unresolveComment { visibility: visible; } -} - -.unresolveComment { - position: absolute; - width: 84px; - top: 23px; - left: 0px; - visibility: hidden; -} - /* jQuery autocomplete styles. */ .ui-widget { /* This needs some width set, but it will fit itself to the size of the autocompleting input box. */ diff --git a/views/_comment.erb b/views/_comment.erb index 2c122f79..e8059d98 100644 --- a/views/_comment.erb +++ b/views/_comment.erb @@ -3,10 +3,8 @@ <% if comment.action_required %> <% if comment.resolved_at.nil? %> <% state = "newComment" %> - <% elsif comment.closed_at.nil? %> - <% state = "resolvedComment" %> <% else %> - <% state = "closedComment" %> + <% state = "resolvedComment" %> <% end %> <% end %>
    @@ -23,16 +21,11 @@
    <% if current_user %> <% if comment.action_required %> - - <% if comment.closed_at %> - - <% elsif comment.resolved_at %> - + <% if comment.resolved_at %> + <% else %> <% end %> - - <% end %> <% if comment.line_number %> From a6509bc55dd40cd0c661f5206c185f36841ad887 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Fri, 25 Jan 2013 10:19:01 -0800 Subject: [PATCH 55/71] Add helper methods to the comment model for getting the state of a comment. --- models/comment.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/models/comment.rb b/models/comment.rb index 3129984e..c50ebeeb 100644 --- a/models/comment.rb +++ b/models/comment.rb @@ -32,6 +32,24 @@ def general_comment?() commit_file_id.nil? end # True if this comment pertains to a particular file. def file_comment?() !commit_file_id.nil? end + def state + return "New" if self.resolved_at.nil? + return "Resolved" if self.closed_at.nil? + "Closed" + end + + def isNew + self.resolved_at.nil? + end + + def isResolved + !self.resolved_at.nil? && self.closed_at.nil? + end + + def isClosed + !self.resolved_at.nil? && !self.closed_at.nil? + end + def resolve self.resolved_at = Time.now.utc self.closed_at = nil From 99b5acea65ed150c9b1a44e72f5e9d33c2e81d4b Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Fri, 25 Jan 2013 10:24:40 -0800 Subject: [PATCH 56/71] Added tags to individual comments and made review lists collapsable. --- barkeep_server.rb | 2 + lib/review_list_entry.rb | 2 + lib/tag_helper.rb | 7 ++++ models/commit.rb | 46 ++++++++--------------- public/coffee/reviews.coffee | 29 +++++++++++++- public/css/saved_search.scss | 20 +++++++--- public/css/styles.scss | 1 - views/_commit_with_comments_entry.erb | 54 ++++++++++++++++++--------- views/_commits_with_comments_list.erb | 14 ++++++- views/_review_request_entry.erb | 2 +- views/_review_request_list.erb | 10 ++++- views/reviews.erb | 11 ++++-- 12 files changed, 136 insertions(+), 62 deletions(-) create mode 100644 lib/tag_helper.rb diff --git a/barkeep_server.rb b/barkeep_server.rb index b6bd1749..d71f7d01 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -35,6 +35,7 @@ require "lib/redis_manager" require "lib/redcarpet_extensions" require "lib/mustache_renderer" +require "lib/tag_helper" require "resque_jobs/deliver_review_request_emails.rb" NODE_MODULES_BIN_PATH = "./node_modules/.bin" @@ -531,6 +532,7 @@ def ensure_required_params(*required_params) :recently_reviewed_commits => recently_reviewed_commits, :requests_from_me => requests_from_me, :review_list_ids => review_list_order, + :current_user_id => current_user.id, } end diff --git a/lib/review_list_entry.rb b/lib/review_list_entry.rb index da6946cf..8e649a3a 100644 --- a/lib/review_list_entry.rb +++ b/lib/review_list_entry.rb @@ -3,9 +3,11 @@ class ReviewListEntry attr_accessor :grit_commit attr_accessor :review_request + attr_accessor :comments def initialize(grit_commit) @grit_commit = grit_commit @review_request = nil + @comments = nil end end diff --git a/lib/tag_helper.rb b/lib/tag_helper.rb new file mode 100644 index 00000000..58491b38 --- /dev/null +++ b/lib/tag_helper.rb @@ -0,0 +1,7 @@ +module TagHelper + def self.get_label_and_class(current_user_id, comment) + label_class = (current_user_id == comment.user_id) ? "fromMe" : "toMe" + label = comment.state + [label_class, label] + end +end diff --git a/models/commit.rb b/models/commit.rb index 1359df84..4d038ca3 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -73,30 +73,27 @@ def self.prefix_match(git_repo, partial_sha, zero_commits_ok = false) end def self.create_review_list_entries(commits) + comments = Hash.new { |hash, key| hash[key] = [] } + commits.each do |commit| + comments[commit[:id]] << Comment[commit[:comment_id]] + end + entries = [] + commit_processed = {} commits.each do |commit| - grit_commit = MetaRepo.instance.grit_commit(commit.git_repo.name, commit.sha) + next if commit_processed[commit[:id]] + commit_processed[commit[:id]] = true + grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) next unless grit_commit - entries << ReviewListEntry.new(grit_commit) + entry = ReviewListEntry.new(grit_commit) + entry.comments = comments[commit[:id]] + entries << entry end entries end - # Fetches the commits with unresolved comments for the given email. The email is used to find - # the commits by that user and to exclude comments made by that user. - def self.commits_with_unresolved_comments(email) - commits = Commit. - join(:comments, :commit_id => :id). - join(:authors, :id => :commits__author_id). - join(:users, :id => :comments__user_id). - filter(:authors__email => email, :comments__resolved_at => nil). - exclude(:users__email => email). - group_by(:commits__id).all - create_review_list_entries(commits) - end - def self.commits_with_recently_resolved_comments(email) - commits = Commit. + commits = Commit.select(:commits__id, :git_repo_id, :sha). join(:comments, :commit_id => :id). join(:authors, :id => :commits__author_id). join(:users, :id => :comments__user_id). @@ -108,17 +105,6 @@ def self.commits_with_recently_resolved_comments(email) create_review_list_entries(commits) end - def self.commits_with_unresolved_comments_from_me(email) - commits = Commit. - join(:comments, :commit_id => :id). - join(:authors, :id => :commits__author_id). - join(:users, :id => :comments__user_id). - filter(:users__email => email, :comments__resolved_at => nil). - exclude(:authors__email => email). - group_by(:commits__id).all - create_review_list_entries(commits) - end - # Selects for the given user all the commits with "actionable" comments, that is, comments that # match the following conditions: # 1. Comments that have "action_required" and are not closed, and @@ -126,13 +112,13 @@ def self.commits_with_unresolved_comments_from_me(email) # and are not resolved (and not closed), or # 2b. Comments that this user made on some commit that are resolved (but not closed). def self.commits_with_actionable_comments_for_user(user_id) - commits = Commit. + commits = Commit.select(:commits__id, :git_repos__name, :sha, :comments__id___comment_id). join(:comments, :commit_id => :id). join(:authors, :id => :commits__author_id). + join(:git_repos, :id => :commits__git_repo_id). filter(:action_required => true, :comments__closed_at => nil). where({ :authors__user_id => user_id, :comments__resolved_at => nil } | - { :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }). - group_by(:commits__id).all + { :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }).all create_review_list_entries(commits) end end diff --git a/public/coffee/reviews.coffee b/public/coffee/reviews.coffee index df8811de..25c4b3c4 100644 --- a/public/coffee/reviews.coffee +++ b/public/coffee/reviews.coffee @@ -1,7 +1,9 @@ window.Reviews = init: -> - $(".review .delete").live "click", (e) => @onReviewComplete e + $(".review .deleteRequestRow").live "click", (e) => @onReviewComplete e + $(".review .deleteCommentRow").live "click", (e) => @onCommentComplete e + $(".review .expandCollapseListIcon").click (e) => @onExpandCollapseList e $("#reviewLists").sortable placeholder: "savedSearchPlaceholder" handle: ".dragHandle" @@ -20,6 +22,31 @@ window.Reviews = url: "/review_lists/reorder" data: state.toString() + onExpandCollapseList: (e) -> + console.log("onCollapseList") + target = $(e.currentTarget) + if target.html() == "+" + target.html("-") + else + target.html("+") + target.parents(".review").find(".reviewListBody").toggleClass("collapsedList") + false + + onCommentComplete: (e) -> + target = $(e.currentTarget) + commentId = target.data("commentId") + $.ajax({ + type: "post", + url: "/close_comment", + data: { + comment_id: commentId + } + success: (html) => + target.parents(".commentRow").remove() + # TODO(jack): if no more comments, then remove commit line too + }) + false + onReviewComplete: (e) -> target = $(e.currentTarget) repo = target.data("repo") diff --git a/public/css/saved_search.scss b/public/css/saved_search.scss index 626bd9ae..b57acb63 100644 --- a/public/css/saved_search.scss +++ b/public/css/saved_search.scss @@ -172,7 +172,7 @@ } #reviewLists { - .headerLabel { + .tagLabel { padding: 1px 4px; font-size: 12px; line-height: 18px; @@ -184,14 +184,24 @@ } .toMe { background: $coreRed; } .fromMe { background: darkgreen; } - a.delete { + a.deleteRequestRow, a.deleteCommentRow { padding-right: 5px; - float: right; color: red; @include closeLinks; } - .tableCellDelete { visibility: hidden; } - #uncompleted_reviews .tableCellDelete { visibility: visible; } + .tableCellDeleteHidden { visibility: hidden; } + .tableCellDelete { visibility: visible; } + span.expandCollapseListIcon { + // TODO(jack): use an image instead of a plus or minus sign + cursor: pointer; + color: navy; + padding: 1px 4px; + font-size: 20px; + font-weight: bold; + border-radius: 3px; + Background: lightgray; + } + div.collapsedList { display: none; } } /* The actual commit search box. */ diff --git a/public/css/styles.scss b/public/css/styles.scss index d265cb9c..71fd4f5d 100644 --- a/public/css/styles.scss +++ b/public/css/styles.scss @@ -467,7 +467,6 @@ table.innerCommitsList { } } - /* * Styles for the stats page. */ diff --git a/views/_commit_with_comments_entry.erb b/views/_commit_with_comments_entry.erb index 66bb4d81..9506aad0 100644 --- a/views/_commit_with_comments_entry.erb +++ b/views/_commit_with_comments_entry.erb @@ -2,12 +2,14 @@ A single table row for a commit with unresolved comments. View arguments: - entry: a ReviewListEntry + - current_user_id: the current user id %> <% commit = entry.grit_commit %> +<% comments = entry.comments %> <% db_commit = MetaRepo.instance.db_commit(commit.repo_name, commit.sha) %> - - X + + X " title="Approved" rel="tipsy"> @@ -20,20 +22,36 @@ <%= commit.author.to_s[0..25] %> - - <%= commit.id_abbrev %> - - - <%= CGI::escapeHTML(commit.short_message) %> - - <%= commit.date.to_pretty %> - <%= commit.repo_name %> - - <% comment_count = db_commit.comment_count %> - <% if comment_count > 0 %> - "> - <%= comment_count %>" /> - - <% end %> - + + + + + + + + + <% comments.each do |comment| %> + + + + <% end %> +
    + <%= commit.id_abbrev %> + + <%= CGI::escapeHTML(commit.short_message) %> + <%= commit.date.to_pretty %><%= commit.repo_name %> + <% comment_count = db_commit.comment_count %> + <% if comment_count > 0 %> + "> + <%= comment_count %>" /> + + <% end %> +
    + + x + + <% label_class, label = TagHelper.get_label_and_class(current_user_id, comment) %> + <%= label %> + <%= comment.text[0..100] %> +
    diff --git a/views/_commits_with_comments_list.erb b/views/_commits_with_comments_list.erb index 7fde43ab..3e0853d5 100644 --- a/views/_commits_with_comments_list.erb +++ b/views/_commits_with_comments_list.erb @@ -6,16 +6,24 @@ - labelClass: the class to use for the label (this determines the css style such as background color) - header: a string that is displayed above the list - review_list: an array where each element is a ReviewListEntry + - current_user_id: the current user id + - collapsed: true if list should be shown in the collapsed state (only header is displayed) %>
    - <%= label %> + <% if collapsed %> + + + <% else %> + - + <% end %> + <%= label %> <%= header %>
    +
    diff --git a/views/_review_request_entry.erb b/views/_review_request_entry.erb index 67298335..7a4a6144 100644 --- a/views/_review_request_entry.erb +++ b/views/_review_request_entry.erb @@ -8,7 +8,7 @@ <% db_commit = MetaRepo.instance.db_commit(commit.repo_name, commit.sha) %> - X + X " title="Approved" rel="tipsy"> diff --git a/views/_review_request_list.erb b/views/_review_request_list.erb index c9ecaa9e..f9382f6e 100644 --- a/views/_review_request_list.erb +++ b/views/_review_request_list.erb @@ -7,16 +7,23 @@ - header: a string that is displayed above the list - review_list: an array where each element is a ReviewListEntry - request_type: either "Requested" or "Completed" + - collapsed: true if list should be shown in the collapsed state (only header is displayed) %>
    - <%= label %> + <% if collapsed %> + + + <% else %> + - + <% end %> + <%= label %> <%= header %>
    +
    diff --git a/views/reviews.erb b/views/reviews.erb index 5040ed7b..c5f4e511 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -6,6 +6,7 @@ <% if list_id == "uncompleted_reviews" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, + :collapsed => false, :label => "For me", :labelClass => "toMe", :header => "Code review requests", @@ -16,15 +17,18 @@ <% if list_id == "actionable_comments" %> <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, - :label => "For me", - :labelClass => "toMe", + :collapsed => false, + :label => "", + :labelClass => "", :header => "Actionable comments", - :review_list => commits_with_actionable_comments } %> + :review_list => commits_with_actionable_comments, + :current_user_id => current_user_id } %> <% end %> <% if list_id == "recent_reviews" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, + :collapsed => true, :label => "For me", :labelClass => "toMe", :header => "Recently completed requests", @@ -35,6 +39,7 @@ <% if list_id == "requests_from_me" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, + :collapsed => true, :label => "From me", :labelClass => "fromMe", :header => "Code review requests", From d83852abd67f7020015de027fc1b5086b7071322 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 27 Jan 2013 08:41:33 -0800 Subject: [PATCH 57/71] Add links to comments in Reviews lists. --- views/_commit_with_comments_entry.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/views/_commit_with_comments_entry.erb b/views/_commit_with_comments_entry.erb index 9506aad0..fcde825c 100644 --- a/views/_commit_with_comments_entry.erb +++ b/views/_commit_with_comments_entry.erb @@ -49,7 +49,7 @@ <% label_class, label = TagHelper.get_label_and_class(current_user_id, comment) %> <%= label %> - <%= comment.text[0..100] %> + <%= comment.text[0..100] %> <% end %> From 34646cc9581a6eba72f59d1a12141f5311f774cc Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 27 Jan 2013 08:42:12 -0800 Subject: [PATCH 58/71] Add lists for pending comments and closed comments. --- barkeep_server.rb | 8 ++++++-- models/commit.rb | 49 +++++++++++++++++++++++++++++++++++++++++------ views/reviews.erb | 22 +++++++++++++++++++++ 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index d71f7d01..885803cb 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -521,16 +521,20 @@ def ensure_required_params(*required_params) get "/reviews" do uncompleted_reviews = ReviewRequest.commits_with_uncompleted_reviews(current_user.id) - actionable_comments = Commit.commits_with_actionable_comments_for_user(current_user.id) + actionable_comments = Commit.commits_with_actionable_comments(current_user.id) + pending_comments = Commit.commits_with_pending_comments(current_user.id) + closed_comments = Commit.commits_with_closed_comments(current_user.id) recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) requests_from_me = ReviewRequest.requests_from_me(current_user.id) - default_list = "uncompleted_reviews,actionable_comments,recent_reviews,recent_comments,requests_from_me,comments_from_me" + default_list = "uncompleted_reviews,actionable_comments,recent_reviews,,requests_from_me,pending_comments,closed_comments" review_list_order = (current_user.review_list_order || default_list).split(",") erb :reviews, :locals => { :commits_with_uncompleted_reviews => uncompleted_reviews, :commits_with_actionable_comments => actionable_comments, :recently_reviewed_commits => recently_reviewed_commits, :requests_from_me => requests_from_me, + :commits_with_pending_comments => pending_comments, + :commits_with_closed_comments => closed_comments, :review_list_ids => review_list_order, :current_user_id => current_user.id, } diff --git a/models/commit.rb b/models/commit.rb index 4d038ca3..b6ecefb4 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -106,12 +106,13 @@ def self.commits_with_recently_resolved_comments(email) end # Selects for the given user all the commits with "actionable" comments, that is, comments that - # match the following conditions: - # 1. Comments that have "action_required" and are not closed, and - # 2a. Comments that were made on one of this user's commits (including any comments by this user), - # and are not resolved (and not closed), or - # 2b. Comments that this user made on some commit that are resolved (but not closed). - def self.commits_with_actionable_comments_for_user(user_id) + # are New and for user, or Resolved and from user. + def self.commits_with_actionable_comments(user_id) + # The SQL query selects comments that match the following conditions: + # 1. Comments that have "action_required" and are not closed, and + # 2a. Comments that were made on one of this user's commits (including any comments by this user), + # and are not resolved (and not closed), or + # 2b. Comments that this user made on some commit that are resolved (but not closed). commits = Commit.select(:commits__id, :git_repos__name, :sha, :comments__id___comment_id). join(:comments, :commit_id => :id). join(:authors, :id => :commits__author_id). @@ -121,4 +122,40 @@ def self.commits_with_actionable_comments_for_user(user_id) { :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }).all create_review_list_entries(commits) end + + # Selects for the given user all the commits with comments waiting on someone else's action, + # that is, comments that are New and from user, or Resolved and for user. + def self.commits_with_pending_comments(user_id) + # The SQL query selects comments that match the following conditions: + # 1. Comments that have "action_required" and are not closed, and + # 2a. Comments that were made on one of this user's commits (including any comments by this user), + # and are resolved, or + # 2b. Comments that this user made on some commit that are new. + commits = Commit.select(:commits__id, :git_repos__name, :sha, :comments__id___comment_id). + join(:comments, :commit_id => :id). + join(:authors, :id => :commits__author_id). + join(:git_repos, :id => :commits__git_repo_id). + filter(:action_required => true, :comments__closed_at => nil). + where({ :comments__user_id => user_id, :comments__resolved_at => nil } | + { :authors__user_id => user_id } & ~{ :comments__resolved_at => nil }).all + create_review_list_entries(commits) + end + + # Selects for the given user all the commits with closed comments related to the given user, + # that is, comments that are Closed and from user, or Closed and for user. + def self.commits_with_closed_comments(user_id) + # The SQL query selects comments that match the following conditions: + # 1. Comments that have "action_required" and are closed, and + # 2a. Comments that were made on one of this user's commits (including any comments by this user), + # or + # 2b. Comments that this user made on some commit. + commits = Commit.select(:commits__id, :git_repos__name, :sha, :comments__id___comment_id). + join(:comments, :commit_id => :id). + join(:authors, :id => :commits__author_id). + join(:git_repos, :id => :commits__git_repo_id). + filter(:action_required => true). + where( ~{ :comments__closed_at => nil } & + ({ :comments__user_id => user_id } | { :authors__user_id => user_id })).all + create_review_list_entries(commits) + end end diff --git a/views/reviews.erb b/views/reviews.erb index c5f4e511..b6879e54 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -47,5 +47,27 @@ :review_list => requests_from_me } %> <% end %> +<% if list_id == "pending_comments" %> + <%= erb :_commits_with_comments_list, :layout => false, :locals => { + :list_id => list_id, + :collapsed => true, + :label => "", + :labelClass => "", + :header => "Pending comments", + :review_list => commits_with_pending_comments, + :current_user_id => current_user_id } %> +<% end %> + +<% if list_id == "closed_comments" %> + <%= erb :_commits_with_comments_list, :layout => false, :locals => { + :list_id => list_id, + :collapsed => true, + :label => "", + :labelClass => "", + :header => "Closed comments", + :review_list => commits_with_closed_comments, + :current_user_id => current_user_id } %> +<% end %> + <% end %>
    From fee8fb59afae6beb2a322fdac7fb33d9e82db00c Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 27 Jan 2013 14:51:56 -0800 Subject: [PATCH 59/71] Hide delete icon in comments list to prevent non-authors from closing resolved comments. --- models/comment.rb | 14 +++++++++++--- views/_commit_with_comments_entry.erb | 2 +- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/models/comment.rb b/models/comment.rb index c50ebeeb..eed65159 100644 --- a/models/comment.rb +++ b/models/comment.rb @@ -38,18 +38,26 @@ def state "Closed" end - def isNew + def is_new self.resolved_at.nil? end - def isResolved + def is_resolved !self.resolved_at.nil? && self.closed_at.nil? end - def isClosed + def is_closed !self.resolved_at.nil? && !self.closed_at.nil? end + def is_for_user(user_id) + self.commit.author.user_id == user_id + end + + def is_by_user(user_id) + self.user_id == user_id + end + def resolve self.resolved_at = Time.now.utc self.closed_at = nil diff --git a/views/_commit_with_comments_entry.erb b/views/_commit_with_comments_entry.erb index fcde825c..e2c901e2 100644 --- a/views/_commit_with_comments_entry.erb +++ b/views/_commit_with_comments_entry.erb @@ -44,7 +44,7 @@ <% comments.each do |comment| %> - + x <% label_class, label = TagHelper.get_label_and_class(current_user_id, comment) %> From 2b0505f7b85f9b819c929f0d378cc1b93b0b48ce Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 27 Jan 2013 14:52:45 -0800 Subject: [PATCH 60/71] Add zebra striping to commit lists with inline comments. --- public/css/saved_search.scss | 1 + views/_commits_with_comments_list.erb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/public/css/saved_search.scss b/public/css/saved_search.scss index b57acb63..10aeb298 100644 --- a/public/css/saved_search.scss +++ b/public/css/saved_search.scss @@ -202,6 +202,7 @@ Background: lightgray; } div.collapsedList { display: none; } + .commitsWithCommentsList > tbody > tr:nth-child(even) { background: #fcf8dc; } } /* The actual commit search box. */ diff --git a/views/_commits_with_comments_list.erb b/views/_commits_with_comments_list.erb index 3e0853d5..6e9b7ff4 100644 --- a/views/_commits_with_comments_list.erb +++ b/views/_commits_with_comments_list.erb @@ -30,7 +30,7 @@ <% else %> - +
    <% review_list.each do |entry| %> <%= erb :_commit_with_comments_entry, :layout => false, :locals => { :entry => entry, :current_user_id => current_user_id } %> From 36ce7e693f18beb1b078a1d522f2d4572e4eebc4 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 27 Jan 2013 15:57:05 -0800 Subject: [PATCH 61/71] Show up to 3 comments, then add 'Show all comments' link. --- public/coffee/reviews.coffee | 8 +++++++- public/css/saved_search.scss | 7 +++++++ views/_commit_with_comments_entry.erb | 13 ++++++++++--- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/public/coffee/reviews.coffee b/public/coffee/reviews.coffee index 25c4b3c4..b3c26185 100644 --- a/public/coffee/reviews.coffee +++ b/public/coffee/reviews.coffee @@ -4,6 +4,7 @@ window.Reviews = $(".review .deleteRequestRow").live "click", (e) => @onReviewComplete e $(".review .deleteCommentRow").live "click", (e) => @onCommentComplete e $(".review .expandCollapseListIcon").click (e) => @onExpandCollapseList e + $(".review .showOverflowComments").click (e) => @onExpandCommentsList e $("#reviewLists").sortable placeholder: "savedSearchPlaceholder" handle: ".dragHandle" @@ -22,8 +23,13 @@ window.Reviews = url: "/review_lists/reorder" data: state.toString() + onExpandCommentsList: (e) -> + target = $(e.currentTarget) + target.parents(".commentEntryRow").find(".overflowCommentRow").show() + target.parents(".overflowButtonRow").hide() + false + onExpandCollapseList: (e) -> - console.log("onCollapseList") target = $(e.currentTarget) if target.html() == "+" target.html("-") diff --git a/public/css/saved_search.scss b/public/css/saved_search.scss index 10aeb298..5983ce62 100644 --- a/public/css/saved_search.scss +++ b/public/css/saved_search.scss @@ -203,6 +203,13 @@ } div.collapsedList { display: none; } .commitsWithCommentsList > tbody > tr:nth-child(even) { background: #fcf8dc; } + .overflowCommentRow { display: none; } + .showOverflowComments { + padding: 2px 4px; + background: lightgrey; + border-radius: 3px; + } + .centerText { text-align: center; } } /* The actual commit search box. */ diff --git a/views/_commit_with_comments_entry.erb b/views/_commit_with_comments_entry.erb index e2c901e2..83b76b8d 100644 --- a/views/_commit_with_comments_entry.erb +++ b/views/_commit_with_comments_entry.erb @@ -7,7 +7,7 @@ <% commit = entry.grit_commit %> <% comments = entry.comments %> <% db_commit = MetaRepo.instance.db_commit(commit.repo_name, commit.sha) %> - + @@ -41,8 +41,8 @@ <% end %> - <% comments.each do |comment| %> - + <% comments.each_with_index do |comment, index| %> + <% end %> + <% if comments.length >= 4 %> + + + + <% end %>
    X
    x @@ -53,5 +53,12 @@
    + Show all <%= comments.length %> comments +
    From da2e8ee261c8015275667d3f808a6e4ed3489ab3 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 24 Feb 2013 21:04:33 -0800 Subject: [PATCH 62/71] Add paging for review lists. --- barkeep_server.rb | 40 ++++- lib/models.rb | 1 + lib/review_list.rb | 42 +++++ models/commit.rb | 219 +++++++++++++++++++++----- models/review_request.rb | 41 ++--- public/coffee/reviews.coffee | 42 ++++- views/_commits_with_comments_list.erb | 17 +- views/_review_request_list.erb | 15 +- views/reviews.erb | 17 +- 9 files changed, 357 insertions(+), 77 deletions(-) create mode 100644 lib/review_list.rb diff --git a/barkeep_server.rb b/barkeep_server.rb index 885803cb..0aa7eb44 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -405,10 +405,11 @@ def ensure_required_params(*required_params) recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) erb :_review_request_list, :layout => false, :locals => { :list_id => "recent_reviews", + :collapsed => true, :label => "For me", :labelClass => "toMe", :recently_reviewed_commits => recently_reviewed_commits, - :header => "My recently completed code review requests", + :header => "Recently completed requests", :request_type => "Completed", :review_list => recently_reviewed_commits } @@ -526,8 +527,7 @@ def ensure_required_params(*required_params) closed_comments = Commit.commits_with_closed_comments(current_user.id) recently_reviewed_commits = ReviewRequest.recently_reviewed_commits(current_user.id) requests_from_me = ReviewRequest.requests_from_me(current_user.id) - default_list = "uncompleted_reviews,actionable_comments,recent_reviews,,requests_from_me,pending_comments,closed_comments" - review_list_order = (current_user.review_list_order || default_list).split(",") + review_list_order = (current_user.review_list_order || ReviewList::DEFAULT_LIST).split(",") erb :reviews, :locals => { :commits_with_uncompleted_reviews => uncompleted_reviews, :commits_with_actionable_comments => actionable_comments, @@ -540,6 +540,40 @@ def ensure_required_params(*required_params) } end + # Gets the next or previous page for a review list + # - name: a name specifying the review list + # - token: a paging token containing info on how to fetch the next or previous page + get "/review_list/:name" do + MetaRepo.instance.scan_for_new_repos + token = params[:token] + direction = params[:direction] + locals = { + :review_list_ids => [params[:name]], + :current_user_id => current_user.id, + } + case params[:name] + when "uncompleted_reviews" + review_list = ReviewRequest.commits_with_uncompleted_reviews(current_user.id, token, direction) + locals[:commits_with_uncompleted_reviews] = review_list + when "actionable_comments" + review_list = Commit.commits_with_actionable_comments(current_user.id, token, direction) + locals[:commits_with_actionable_comments] = review_list + when "pending_comments" + review_list = Commit.commits_with_pending_comments(current_user.id, token, direction) + locals[:commits_with_pending_comments] = review_list + when "closed_comments" + review_list = Commit.commits_with_closed_comments(current_user.id, token, direction) + locals[:commits_with_closed_comments] = review_list + when "recent_reviews" + review_list = ReviewRequest.recently_reviewed_commits(current_user.id, token, direction) + locals[:recently_reviewed_commits] = review_list + when "requests_from_me" + review_list = ReviewRequest.requests_from_me(current_user.id, token, direction) + locals[:requests_from_me] = review_list + end + erb :reviews, :layout => false, :locals => locals + end + post "/review_lists/reorder" do review_list_ids = request.body.read User.filter(:id => current_user.id). diff --git a/lib/models.rb b/lib/models.rb index fb4b633a..d2c5b082 100644 --- a/lib/models.rb +++ b/lib/models.rb @@ -21,4 +21,5 @@ require "models/comment" require "models/completed_email" require "models/review_request" +require "lib/review_list" require "lib/review_list_entry" diff --git a/lib/review_list.rb b/lib/review_list.rb new file mode 100644 index 00000000..d17cb760 --- /dev/null +++ b/lib/review_list.rb @@ -0,0 +1,42 @@ +# This class encapsulates the data for a "review list" on the "Reviews" page. + +class ReviewList + attr_accessor :entries + attr_accessor :token + + DEFAULT_LIST = "uncompleted_reviews,actionable_comments,recent_reviews,requests_from_me," + + "pending_comments,closed_comments" + + def initialize(entries, token) + @entries = entries + @token = token + end + + def page_number + @token.split(";", 2)[0].to_i + end + + # A token contains the following values, separated by a semi-colon: + # page_number ; from_values ; to_values ; is_partial + # where + # page_number is the page number of the current page + # is_partial is "true" if the list is only partially filled + # from_values is either the first commit id in the list, or a comma-separated pair + # of values (commit_id, date) + # to_values is either the last commit id in the list, or a comma-separated pair + # of values (commit_id, date) + def self.parse_token(token) + page_number, from_values, to_values, is_partial = token.split(";") + from_values = from_values.split(",") + from_values[0] = from_values[0].to_i + to_values = to_values.split(",") + to_values[0] = to_values[0].to_i + [page_number.to_i, from_values, to_values, (is_partial == "true")] + end + + def self.make_token(page_number, from_values, to_values, is_partial) + from_values = Array(from_values) + to_values = Array(to_values) + "#{page_number};#{from_values.join(',')};#{to_values.join(',')};#{is_partial}" + end +end diff --git a/models/commit.rb b/models/commit.rb index b6ecefb4..7944ebb9 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -18,6 +18,8 @@ class Commit < Sequel::Model add_association_dependencies :comments => :destroy, :commit_files => :destroy + PAGE_SIZE = 8 + add_filter(:message) { |message| StringFilter.escape_html(message) } add_filter(:message) do |message, commit| StringFilter.replace_shas_with_links(message, commit.git_repo.name, :skip_markdown => true) @@ -72,90 +74,227 @@ def self.prefix_match(git_repo, partial_sha, zero_commits_ok = false) end end - def self.create_review_list_entries(commits) - comments = Hash.new { |hash, key| hash[key] = [] } - commits.each do |commit| - comments[commit[:id]] << Comment[commit[:comment_id]] - end + def self.opposite_sort_order(order) + (order == :asc) ? :desc : :asc + end - entries = [] - commit_processed = {} - commits.each do |commit| - next if commit_processed[commit[:id]] - commit_processed[commit[:id]] = true - grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) - next unless grit_commit - entry = ReviewListEntry.new(grit_commit) - entry.comments = comments[commit[:id]] - entries << entry + def self.fetch_paged_rows(args) + dataset = args[:dataset] + major_col = args[:major_col] + minor_col = args[:minor_col] + major_sort = args[:major_sort] + minor_sort = args[:minor_sort] + major_op = args[:major_op] + minor_op = args[:minor_op] + values = args[:values] + group_by_col = args[:group_by_col] + page_size = args[:page_size] + if major_col != minor_col + if values[1].to_s != "0" + conditions = "#{major_col} #{major_op} ? or (#{major_col} = ? and #{minor_col} #{minor_op} ?)" + dataset = dataset.filter(conditions, values[1], values[1], values[0]) + end + dataset = dataset.order(Sequel.send(major_sort, major_col)). + order_append(Sequel.send(minor_sort, minor_col)) + else + dataset = dataset.filter("#{minor_col} #{minor_op} ?", values[0]). + order(Sequel.send(minor_sort, minor_col)) end - entries + rows = dataset.group_by(group_by_col).limit(page_size).all end - def self.commits_with_recently_resolved_comments(email) - commits = Commit.select(:commits__id, :git_repo_id, :sha). - join(:comments, :commit_id => :id). - join(:authors, :id => :commits__author_id). - join(:users, :id => :comments__user_id). - filter(:authors__email => email). - exclude(:comments__resolved_at => nil). - exclude(:users__email => email). - group_by(:commits__id). - reverse_order(:resolved_at).limit(5).all - create_review_list_entries(commits) + def self.paginate_dataset(dataset, page_by_cols, group_by_col, token, direction, page_size) + page_number, from_values, to_values, is_partial = ReviewList.parse_token(token) + if page_by_cols.length != from_values.length || page_by_cols.length != to_values.length + raise ArgumentError.new "page_by_cols.length (#{page_by_cols.length}) does not match token" + end + major_col, minor_col = page_by_cols + major_col, major_sort = Array(major_col) + minor_col = major_col if minor_col.nil? + major_sort = :asc if major_sort.nil? + minor_sort = :asc + args = { :dataset => dataset, :major_col => major_col, :minor_col => minor_col, + :major_sort => major_sort, :minor_sort => minor_sort, + :group_by => group_by_col, :page_size => page_size } + if direction == "next" + if is_partial + args[:major_op] = :< + args[:minor_op] = :>= + args[:values] = from_values + else + args[:major_op] = :< + args[:minor_op] = :> + args[:values] = to_values + page_number += 1 + end + rows = fetch_paged_rows(args) + + if rows.empty? + page_number -= 1 + # There is no "next" page, so refill the current page, searching backwards from the + # last commit id on this page. + # This also covers the case where there was a partial page but it is now empty. + args[:major_op] = :> + args[:minor_op] = :<= + args[:values] = to_values + args[:major_sort] = opposite_sort_order(major_sort) + args[:minor_sort] = opposite_sort_order(minor_sort) + rows = fetch_paged_rows(args) + rows.reverse! + end + else + args[:major_sort] = opposite_sort_order(major_sort) + args[:minor_sort] = opposite_sort_order(minor_sort) + args[:major_op] = :> + args[:minor_op] = :< + args[:values] = from_values + rows = fetch_paged_rows(args) + + if rows.length < page_size + # There is no "prev" page or the prev page was not full, so reset the page number to 1, and + # refill it from the beginning. + args[:major_sort] = major_sort + args[:minor_sort] = minor_sort + args[:major_op] = :> + args[:minor_op] = :> + args[:values] = [0, 0] + rows = fetch_paged_rows(args) + page_number = 1 + else + rows.reverse! + page_number -= 1 + page_number = 1 if page_number < 1 + end + end + if rows.empty? + is_partial = false + from_values = [0, 0] + to_values = [0, 0] + else + is_partial = rows.length < page_size + if major_col == minor_col + from_values = [rows.first[minor_col]] + to_values = [rows.last[minor_col]] + else + from_values = [rows.first[minor_col], rows.first[major_col]] + to_values = [rows.last[minor_col], rows.last[major_col]] + end + end + token = ReviewList.make_token(page_number, from_values, to_values, is_partial) + [rows, token] end - # Selects for the given user all the commits with "actionable" comments, that is, comments that + # Selects for the given user the commits with "actionable" comments, that is, comments that # are New and for user, or Resolved and from user. - def self.commits_with_actionable_comments(user_id) + def self.commits_with_actionable_comments(user_id, token = nil, direction = "next", page_size = PAGE_SIZE) # The SQL query selects comments that match the following conditions: # 1. Comments that have "action_required" and are not closed, and # 2a. Comments that were made on one of this user's commits (including any comments by this user), # and are not resolved (and not closed), or # 2b. Comments that this user made on some commit that are resolved (but not closed). - commits = Commit.select(:commits__id, :git_repos__name, :sha, :comments__id___comment_id). + token = ReviewList.make_token(0, 0, 0, false) if token.nil? + dataset = Commit.select(:commits__id, :git_repos__name, :sha, :authors__user_id___authors_user_id). join(:comments, :commit_id => :id). join(:authors, :id => :commits__author_id). join(:git_repos, :id => :commits__git_repo_id). filter(:action_required => true, :comments__closed_at => nil). where({ :authors__user_id => user_id, :comments__resolved_at => nil } | - { :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }).all - create_review_list_entries(commits) + { :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }) + + commits, token = paginate_dataset(dataset, ["commits.id"], "commits.id", token, direction, page_size) + entries = [] + commits.each do |commit| + grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) + next unless grit_commit + if commit[:authors_user_id] == user_id + comments = Comment.filter(:commit_id => commit[:id]). + filter(:action_required => true, :comments__closed_at => nil). + where({ :comments__resolved_at => nil } | { :comments__user_id => user_id }).all + else + comments = Comment.filter(:commit_id => commit[:id]). + filter(:action_required => true, :comments__closed_at => nil). + where({ :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }).all + end + entry = ReviewListEntry.new(grit_commit) + entry.comments = comments + entries << entry + end + ReviewList.new(entries, token) end # Selects for the given user all the commits with comments waiting on someone else's action, # that is, comments that are New and from user, or Resolved and for user. - def self.commits_with_pending_comments(user_id) + def self.commits_with_pending_comments(user_id, token = nil, direction = "next", page_size = PAGE_SIZE) # The SQL query selects comments that match the following conditions: # 1. Comments that have "action_required" and are not closed, and # 2a. Comments that were made on one of this user's commits (including any comments by this user), # and are resolved, or # 2b. Comments that this user made on some commit that are new. - commits = Commit.select(:commits__id, :git_repos__name, :sha, :comments__id___comment_id). + token = ReviewList.make_token(0, 0, 0, false) if token.nil? + dataset = Commit.select(:commits__id, :git_repos__name, :sha, :authors__user_id___authors_user_id). join(:comments, :commit_id => :id). join(:authors, :id => :commits__author_id). join(:git_repos, :id => :commits__git_repo_id). filter(:action_required => true, :comments__closed_at => nil). where({ :comments__user_id => user_id, :comments__resolved_at => nil } | - { :authors__user_id => user_id } & ~{ :comments__resolved_at => nil }).all - create_review_list_entries(commits) + { :authors__user_id => user_id } & ~{ :comments__resolved_at => nil }) + + commits, token = paginate_dataset(dataset, ["commits.id"], "commits.id", token, direction, page_size) + entries = [] + commits.each do |commit| + grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) + next unless grit_commit + if commit[:authors_user_id] == user_id + comments = Comment.filter(:commit_id => commit[:id]). + filter(:action_required => true, :comments__closed_at => nil). + where(~{ :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }).all + else + comments = Comment.filter(:commit_id => commit[:id]). + filter(:action_required => true, :comments__closed_at => nil). + filter(:comments__resolved_at => nil, :comments__user_id => user_id).all + end + entry = ReviewListEntry.new(grit_commit) + entry.comments = comments + entries << entry + end + ReviewList.new(entries, token) end # Selects for the given user all the commits with closed comments related to the given user, # that is, comments that are Closed and from user, or Closed and for user. - def self.commits_with_closed_comments(user_id) + def self.commits_with_closed_comments(user_id, token = nil, direction = "next", page_size = PAGE_SIZE) # The SQL query selects comments that match the following conditions: # 1. Comments that have "action_required" and are closed, and # 2a. Comments that were made on one of this user's commits (including any comments by this user), # or # 2b. Comments that this user made on some commit. - commits = Commit.select(:commits__id, :git_repos__name, :sha, :comments__id___comment_id). + token = ReviewList.make_token(0, 0, 0, false) if token.nil? + dataset = Commit.select(:commits__id, :git_repos__name, :sha, :comments__id___comment_id). join(:comments, :commit_id => :id). join(:authors, :id => :commits__author_id). join(:git_repos, :id => :commits__git_repo_id). filter(:action_required => true). where( ~{ :comments__closed_at => nil } & - ({ :comments__user_id => user_id } | { :authors__user_id => user_id })).all - create_review_list_entries(commits) + ({ :comments__user_id => user_id } | { :authors__user_id => user_id })) + + commits, token = paginate_dataset(dataset, ["commits.id"], "commits.id", token, direction, page_size) + entries = [] + commits.each do |commit| + grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) + next unless grit_commit + if commit[:authors_user_id] == user_id + comments = Comment.filter(:commit_id => commit[:id]). + filter(:action_required => true). + where(~{ :comments__closed_at => nil }).all + else + comments = Comment.filter(:commit_id => commit[:id]). + filter(:action_required => true). + where(~{ :comments__closed_at => nil } & { :comments__user_id => user_id }).all + end + entry = ReviewListEntry.new(grit_commit) + entry.comments = comments + entries << entry + end + ReviewList.new(entries, token) end end diff --git a/models/review_request.rb b/models/review_request.rb index 2455efd0..785abdd4 100644 --- a/models/review_request.rb +++ b/models/review_request.rb @@ -3,7 +3,9 @@ class ReviewRequest < Sequel::Model many_to_one :requester_user, :class => User many_to_one :reviewer_user, :class => User - def self.create_review_list_entries(reviews) + PAGE_SIZE = 8 + + def self.create_review_list_entries(reviews, token) entries = [] reviews.each do |review| grit_commit = MetaRepo.instance.grit_commit(review.commit.git_repo.name, review.commit.sha) @@ -12,30 +14,33 @@ def self.create_review_list_entries(reviews) entry.review_request = review entries << entry end - entries + ReviewList.new(entries, token) end - def self.commits_with_uncompleted_reviews(user_id) - uncompleted = ReviewRequest.filter(:reviewer_user_id => user_id, :completed_at => nil). - group_by(:commit_id).all - create_review_list_entries(uncompleted) + def self.commits_with_uncompleted_reviews(user_id, token = nil, direction = "next", page_size = PAGE_SIZE) + token = ReviewList.make_token(0, 0, 0, false) if token.nil? + dataset = ReviewRequest.filter(:reviewer_user_id => user_id, :completed_at => nil) + reviews, token = Commit.paginate_dataset(dataset, [:id], :commit_id, token, direction, page_size) + create_review_list_entries(reviews, token) end - def self.recently_reviewed_commits(user_id) - recently_reviewed = ReviewRequest.filter(:reviewer_user_id => user_id). - exclude(:completed_at => nil). - group_by(:commit_id). - reverse_order(:completed_at).limit(5).all - create_review_list_entries(recently_reviewed) + def self.recently_reviewed_commits(user_id, token = nil, direction = "next", page_size = PAGE_SIZE) + token = ReviewList.make_token(0, [0, 0], [0, 0], false) if token.nil? + dataset = ReviewRequest.filter(:reviewer_user_id => user_id). + exclude(:completed_at => nil) + reviews, token = Commit.paginate_dataset(dataset, [[:completed_at, :desc], :id], :commit_id, + token, direction, page_size) + create_review_list_entries(reviews, token) end - def self.requests_from_me(user_id) - uncompleted = ReviewRequest.filter(:requester_user_id => user_id, :completed_at => nil). - group_by(:commit_id).all - create_review_list_entries(uncompleted) + def self.requests_from_me(user_id, token = nil, direction = "next", page_size = PAGE_SIZE) + token = ReviewList.make_token(0, 0, 0, false) if token.nil? + dataset = ReviewRequest.filter(:requester_user_id => user_id, :completed_at => nil) + reviews, token = Commit.paginate_dataset(dataset, [:id], :commit_id, token, direction, page_size) + create_review_list_entries(reviews, token) end - def self.complete_requests(commit_id) - ReviewRequest.filter(:commit_id => commit_id).update(:completed_at => Time.now.utc) + def self.complete_requests(commit_ids) + ReviewRequest.filter(:commit_id => commit_ids).update(:completed_at => Time.now.utc) end end diff --git a/public/coffee/reviews.coffee b/public/coffee/reviews.coffee index b3c26185..ebf29b2b 100644 --- a/public/coffee/reviews.coffee +++ b/public/coffee/reviews.coffee @@ -3,8 +3,10 @@ window.Reviews = init: -> $(".review .deleteRequestRow").live "click", (e) => @onReviewComplete e $(".review .deleteCommentRow").live "click", (e) => @onCommentComplete e - $(".review .expandCollapseListIcon").click (e) => @onExpandCollapseList e - $(".review .showOverflowComments").click (e) => @onExpandCommentsList e + $(".review .expandCollapseListIcon").live "click", (e) => @onExpandCollapseList e + $(".review .showOverflowComments").live "click", (e) => @onExpandCommentsList e + $(".review .pageLeftButton").live "click", (e) => @showNextPage e, "prev" + $(".review .pageRightButton").live "click", (e) => @showNextPage e, "next" $("#reviewLists").sortable placeholder: "savedSearchPlaceholder" handle: ".dragHandle" @@ -73,4 +75,40 @@ window.Reviews = }) false + # Shows the next or previous page + showNextPage: (e, direction) -> + return if @fetching + @fetching = true + animationComplete = false + fetchedHtml = null + target = $(e.currentTarget) + token = target.parents(".pageControls").data("token") + reviewList = target.parents(".review") + name = reviewList.attr("id") + tableElement = reviewList.find(".commitsList") + + # We're going to animate sliding the current page away, while at the same time fetching the new page. + # When both of those events are done, showFetchedPage can then be called. + showFetchedPage = => + return unless animationComplete and fetchedHtml + @fetching = false + newReviewList = $(fetchedHtml) + newReviewList.find(".commitsList").css("opacity", 0) + reviewList.replaceWith newReviewList + newReviewList.find(".commitsList").animate({ "opacity": 1 }, { duration: 150 }) + + animateTo = (if direction == "next" then -1 else 1) * tableElement.width() + tableElement.animate { "margin-left": animateTo }, + duration: 400, + complete: => + animationComplete = true + showFetchedPage() + + $.ajax + url: "/review_list/#{name}", + data: { token: token, direction: direction }, + success: (html) => + fetchedHtml = html + showFetchedPage() + $(document).ready(-> Reviews.init()) diff --git a/views/_commits_with_comments_list.erb b/views/_commits_with_comments_list.erb index 6e9b7ff4..25b91684 100644 --- a/views/_commits_with_comments_list.erb +++ b/views/_commits_with_comments_list.erb @@ -5,11 +5,14 @@ - label: a label in the header, such as "For me" - labelClass: the class to use for the label (this determines the css style such as background color) - header: a string that is displayed above the list - - review_list: an array where each element is a ReviewListEntry - - current_user_id: the current user id + - review_list: a ReviewList containing an array of ReviewListEntry's + - current_user_id: the current user id, passed to the next erb file - collapsed: true if list should be shown in the collapsed state (only header is displayed) %> +<% entries = review_list.entries %> +<% token = review_list.token %> +<% page_number = review_list.page_number %>
    @@ -24,18 +27,24 @@
    diff --git a/views/_review_request_list.erb b/views/_review_request_list.erb index f9382f6e..dee104b2 100644 --- a/views/_review_request_list.erb +++ b/views/_review_request_list.erb @@ -5,11 +5,14 @@ - label: a label in the header, such as "For me" - labelClass: the class to use for the label (this determines the css style such as background color) - header: a string that is displayed above the list - - review_list: an array where each element is a ReviewListEntry + - review_list: a ReviewList containing an array of ReviewListEntry's - request_type: either "Requested" or "Completed" - collapsed: true if list should be shown in the collapsed state (only header is displayed) %> +<% entries = review_list.entries %> +<% token = review_list.token %> +<% page_number = review_list.page_number %>
    @@ -24,14 +27,14 @@
    diff --git a/views/reviews.erb b/views/reviews.erb index b6879e54..c9692ccd 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -1,6 +1,9 @@ -<%= js_bundle(:reviews_app_js) %> +<% # If there is just one review list, it must be a snippet %> +<% if review_list_ids.length > 1 %> + <%= js_bundle(:reviews_app_js) %> +
    +<% end %> -
    <% review_list_ids.each do |list_id| %> <% if list_id == "uncompleted_reviews" %> @@ -28,7 +31,7 @@ <% if list_id == "recent_reviews" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, - :collapsed => true, + :collapsed => review_list_ids.length > 1, :label => "For me", :labelClass => "toMe", :header => "Recently completed requests", @@ -39,7 +42,7 @@ <% if list_id == "requests_from_me" %> <%= erb :_review_request_list, :layout => false, :locals => { :list_id => list_id, - :collapsed => true, + :collapsed => review_list_ids.length > 1, :label => "From me", :labelClass => "fromMe", :header => "Code review requests", @@ -50,7 +53,7 @@ <% if list_id == "pending_comments" %> <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, - :collapsed => true, + :collapsed => review_list_ids.length > 1, :label => "", :labelClass => "", :header => "Pending comments", @@ -61,7 +64,7 @@ <% if list_id == "closed_comments" %> <%= erb :_commits_with_comments_list, :layout => false, :locals => { :list_id => list_id, - :collapsed => true, + :collapsed => review_list_ids.length > 1, :label => "", :labelClass => "", :header => "Closed comments", @@ -70,4 +73,4 @@ <% end %> <% end %> -
    +<%= review_list_ids.length > 1 ? "
    " : "" %> From d3ac705dcccac869fe404399e14581eba2f8abab Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Thu, 28 Feb 2013 05:59:32 -0800 Subject: [PATCH 63/71] Add indexes to review_requests table. --- ...37_add_indexes_to_review_requests_table.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 migrations/20130228055037_add_indexes_to_review_requests_table.rb diff --git a/migrations/20130228055037_add_indexes_to_review_requests_table.rb b/migrations/20130228055037_add_indexes_to_review_requests_table.rb new file mode 100644 index 00000000..138cd592 --- /dev/null +++ b/migrations/20130228055037_add_indexes_to_review_requests_table.rb @@ -0,0 +1,19 @@ +require "bundler/setup" +require "pathological" +require "migrations/migration_helper.rb" + +Sequel.migration do + up do + alter_table(:review_requests) do + add_index [:reviewer_user_id, :completed_at] + add_index [:requester_user_id, :completed_at] + end + end + + down do + alter_table(:review_requests) do + drop_index [:reviewer_user_id, :completed_at] + drop_index [:requester_user_id, :completed_at] + end + end +end From 54592ffeeeb23a9c1999e266e0771cfea8568942 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Thu, 28 Feb 2013 06:40:57 -0800 Subject: [PATCH 64/71] Add index for [action_required, closed_at] to comments table. --- ...0130228063626_add_index_to_comments_table.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 migrations/20130228063626_add_index_to_comments_table.rb diff --git a/migrations/20130228063626_add_index_to_comments_table.rb b/migrations/20130228063626_add_index_to_comments_table.rb new file mode 100644 index 00000000..f8241f0e --- /dev/null +++ b/migrations/20130228063626_add_index_to_comments_table.rb @@ -0,0 +1,17 @@ +require "bundler/setup" +require "pathological" +require "migrations/migration_helper.rb" + +Sequel.migration do + up do + alter_table(:comments) do + add_index [:action_required, :closed_at] + end + end + + down do + alter_table(:comments) do + drop_index [:action_required, :closed_at] + end + end +end From 8b020ad467ce5226be1ca462ead19b280ff1ce92 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Fri, 1 Mar 2013 19:31:38 -0800 Subject: [PATCH 65/71] Highlight the commit row above the list of comments. --- public/css/saved_search.scss | 2 +- views/_commit_with_comments_entry.erb | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/public/css/saved_search.scss b/public/css/saved_search.scss index 5983ce62..a7f550f4 100644 --- a/public/css/saved_search.scss +++ b/public/css/saved_search.scss @@ -202,7 +202,7 @@ Background: lightgray; } div.collapsedList { display: none; } - .commitsWithCommentsList > tbody > tr:nth-child(even) { background: #fcf8dc; } + .commitRow { background: #fcf8dc; } .overflowCommentRow { display: none; } .showOverflowComments { padding: 2px 4px; diff --git a/views/_commit_with_comments_entry.erb b/views/_commit_with_comments_entry.erb index 83b76b8d..6574e50e 100644 --- a/views/_commit_with_comments_entry.erb +++ b/views/_commit_with_comments_entry.erb @@ -23,7 +23,7 @@ title="<%= commit.author.email %>"><%= commit.author.to_s[0..25] %> - + @@ -60,5 +60,6 @@ <% end %> +
    <%= commit.id_abbrev %>
     
    From ec714925f0bf29b7b122028162a45f01ec30ce87 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Fri, 1 Mar 2013 20:46:24 -0800 Subject: [PATCH 66/71] Remove commit line in actionable comments when all comments are closed. --- public/coffee/reviews.coffee | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/public/coffee/reviews.coffee b/public/coffee/reviews.coffee index ebf29b2b..44453637 100644 --- a/public/coffee/reviews.coffee +++ b/public/coffee/reviews.coffee @@ -50,8 +50,12 @@ window.Reviews = comment_id: commentId } success: (html) => + table = target.parents(".innerCommitsList") + count = table.find(".commentRow").size() target.parents(".commentRow").remove() - # TODO(jack): if no more comments, then remove commit line too + # if that was the last comment, then remove the commit line too + if count == 1 + table.parents(".commentEntryRow").remove() }) false From 440ae6b11e321e90a3796502bbddad6ef9962f4a Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Fri, 1 Mar 2013 21:17:53 -0800 Subject: [PATCH 67/71] Fix bug with duplicate rows in review lists and simplify paginate_dataset(). --- models/commit.rb | 23 ++++++++++++----------- models/review_request.rb | 14 +++++++------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/models/commit.rb b/models/commit.rb index 7944ebb9..2bb91d9e 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -87,7 +87,6 @@ def self.fetch_paged_rows(args) major_op = args[:major_op] minor_op = args[:minor_op] values = args[:values] - group_by_col = args[:group_by_col] page_size = args[:page_size] if major_col != minor_col if values[1].to_s != "0" @@ -100,10 +99,10 @@ def self.fetch_paged_rows(args) dataset = dataset.filter("#{minor_col} #{minor_op} ?", values[0]). order(Sequel.send(minor_sort, minor_col)) end - rows = dataset.group_by(group_by_col).limit(page_size).all + rows = dataset.limit(page_size).all end - def self.paginate_dataset(dataset, page_by_cols, group_by_col, token, direction, page_size) + def self.paginate_dataset(dataset, page_by_cols, token, direction, page_size) page_number, from_values, to_values, is_partial = ReviewList.parse_token(token) if page_by_cols.length != from_values.length || page_by_cols.length != to_values.length raise ArgumentError.new "page_by_cols.length (#{page_by_cols.length}) does not match token" @@ -114,8 +113,7 @@ def self.paginate_dataset(dataset, page_by_cols, group_by_col, token, direction, major_sort = :asc if major_sort.nil? minor_sort = :asc args = { :dataset => dataset, :major_col => major_col, :minor_col => minor_col, - :major_sort => major_sort, :minor_sort => minor_sort, - :group_by => group_by_col, :page_size => page_size } + :major_sort => major_sort, :minor_sort => minor_sort, :page_size => page_size } if direction == "next" if is_partial args[:major_op] = :< @@ -199,9 +197,10 @@ def self.commits_with_actionable_comments(user_id, token = nil, direction = "nex join(:git_repos, :id => :commits__git_repo_id). filter(:action_required => true, :comments__closed_at => nil). where({ :authors__user_id => user_id, :comments__resolved_at => nil } | - { :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }) + { :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }). + group_by(:commits__id) - commits, token = paginate_dataset(dataset, ["commits.id"], "commits.id", token, direction, page_size) + commits, token = paginate_dataset(dataset, ["commits.id"], token, direction, page_size) entries = [] commits.each do |commit| grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) @@ -237,9 +236,10 @@ def self.commits_with_pending_comments(user_id, token = nil, direction = "next", join(:git_repos, :id => :commits__git_repo_id). filter(:action_required => true, :comments__closed_at => nil). where({ :comments__user_id => user_id, :comments__resolved_at => nil } | - { :authors__user_id => user_id } & ~{ :comments__resolved_at => nil }) + { :authors__user_id => user_id } & ~{ :comments__resolved_at => nil }). + group_by(:commits__id) - commits, token = paginate_dataset(dataset, ["commits.id"], "commits.id", token, direction, page_size) + commits, token = paginate_dataset(dataset, ["commits.id"], token, direction, page_size) entries = [] commits.each do |commit| grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) @@ -275,9 +275,10 @@ def self.commits_with_closed_comments(user_id, token = nil, direction = "next", join(:git_repos, :id => :commits__git_repo_id). filter(:action_required => true). where( ~{ :comments__closed_at => nil } & - ({ :comments__user_id => user_id } | { :authors__user_id => user_id })) + ({ :comments__user_id => user_id } | { :authors__user_id => user_id })). + group_by(:commits__id) - commits, token = paginate_dataset(dataset, ["commits.id"], "commits.id", token, direction, page_size) + commits, token = paginate_dataset(dataset, ["commits.id"], token, direction, page_size) entries = [] commits.each do |commit| grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) diff --git a/models/review_request.rb b/models/review_request.rb index 785abdd4..63dd738e 100644 --- a/models/review_request.rb +++ b/models/review_request.rb @@ -19,24 +19,24 @@ def self.create_review_list_entries(reviews, token) def self.commits_with_uncompleted_reviews(user_id, token = nil, direction = "next", page_size = PAGE_SIZE) token = ReviewList.make_token(0, 0, 0, false) if token.nil? - dataset = ReviewRequest.filter(:reviewer_user_id => user_id, :completed_at => nil) - reviews, token = Commit.paginate_dataset(dataset, [:id], :commit_id, token, direction, page_size) + dataset = ReviewRequest.filter(:reviewer_user_id => user_id, :completed_at => nil).group_by(:commit_id) + reviews, token = Commit.paginate_dataset(dataset, [:id], token, direction, page_size) create_review_list_entries(reviews, token) end def self.recently_reviewed_commits(user_id, token = nil, direction = "next", page_size = PAGE_SIZE) token = ReviewList.make_token(0, [0, 0], [0, 0], false) if token.nil? - dataset = ReviewRequest.filter(:reviewer_user_id => user_id). - exclude(:completed_at => nil) - reviews, token = Commit.paginate_dataset(dataset, [[:completed_at, :desc], :id], :commit_id, + dataset = ReviewRequest.filter(:reviewer_user_id => user_id).exclude(:completed_at => nil). + group_by(:commit_id) + reviews, token = Commit.paginate_dataset(dataset, [[:completed_at, :desc], :id], token, direction, page_size) create_review_list_entries(reviews, token) end def self.requests_from_me(user_id, token = nil, direction = "next", page_size = PAGE_SIZE) token = ReviewList.make_token(0, 0, 0, false) if token.nil? - dataset = ReviewRequest.filter(:requester_user_id => user_id, :completed_at => nil) - reviews, token = Commit.paginate_dataset(dataset, [:id], :commit_id, token, direction, page_size) + dataset = ReviewRequest.filter(:requester_user_id => user_id, :completed_at => nil).group_by(:commit_id) + reviews, token = Commit.paginate_dataset(dataset, [:id], token, direction, page_size) create_review_list_entries(reviews, token) end From 71e56589e125343ea7c1b252430823cc3d287def Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Mon, 4 Mar 2013 10:13:55 -0800 Subject: [PATCH 68/71] Fix bug with paging. --- models/commit.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/models/commit.rb b/models/commit.rb index 2bb91d9e..4c0b8d52 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -96,7 +96,7 @@ def self.fetch_paged_rows(args) dataset = dataset.order(Sequel.send(major_sort, major_col)). order_append(Sequel.send(minor_sort, minor_col)) else - dataset = dataset.filter("#{minor_col} #{minor_op} ?", values[0]). + dataset = dataset.filter("? #{minor_op} ?", minor_col, values[0]). order(Sequel.send(minor_sort, minor_col)) end rows = dataset.limit(page_size).all @@ -170,6 +170,9 @@ def self.paginate_dataset(dataset, page_by_cols, token, direction, page_size) to_values = [0, 0] else is_partial = rows.length < page_size + # Strip off the table name, if any + major_col = major_col.to_s.split("__").last.to_sym + minor_col = minor_col.to_s.split("__").last.to_sym if major_col == minor_col from_values = [rows.first[minor_col]] to_values = [rows.last[minor_col]] @@ -200,7 +203,7 @@ def self.commits_with_actionable_comments(user_id, token = nil, direction = "nex { :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }). group_by(:commits__id) - commits, token = paginate_dataset(dataset, ["commits.id"], token, direction, page_size) + commits, token = paginate_dataset(dataset, [:commits__id], token, direction, page_size) entries = [] commits.each do |commit| grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) @@ -239,7 +242,7 @@ def self.commits_with_pending_comments(user_id, token = nil, direction = "next", { :authors__user_id => user_id } & ~{ :comments__resolved_at => nil }). group_by(:commits__id) - commits, token = paginate_dataset(dataset, ["commits.id"], token, direction, page_size) + commits, token = paginate_dataset(dataset, [:commits__id], token, direction, page_size) entries = [] commits.each do |commit| grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) @@ -278,7 +281,7 @@ def self.commits_with_closed_comments(user_id, token = nil, direction = "next", ({ :comments__user_id => user_id } | { :authors__user_id => user_id })). group_by(:commits__id) - commits, token = paginate_dataset(dataset, ["commits.id"], token, direction, page_size) + commits, token = paginate_dataset(dataset, [:commits__id], token, direction, page_size) entries = [] commits.each do |commit| grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) From a0c7ada16372036f61efb420861ef6c70b3b01ff Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Fri, 8 Mar 2013 22:07:49 -0800 Subject: [PATCH 69/71] Make expanding comment lists work correctly. --- barkeep_server.rb | 11 ++++++++++- models/commit.rb | 18 ++++++++++++++++++ public/coffee/reviews.coffee | 14 +++++++------- public/css/saved_search.scss | 2 +- views/_commit_with_comments_entry.erb | 6 ++++-- views/_commits_with_comments_list.erb | 3 ++- views/reviews.erb | 3 +++ 7 files changed, 45 insertions(+), 12 deletions(-) diff --git a/barkeep_server.rb b/barkeep_server.rb index 0aa7eb44..9260be11 100644 --- a/barkeep_server.rb +++ b/barkeep_server.rb @@ -376,10 +376,17 @@ def ensure_required_params(*required_params) end post "/close_comment" do + expand_comments = (params[:expand_comments] == "true") comment = validate_comment(params[:comment_id], true) comment.close comment.save - erb :_comment, :layout => false, :locals => { :comment => comment } + entry = Commit.actionable_comments(comment.commit, current_user.id) + if entry.nil? + nil + else + erb :_commit_with_comments_entry, :layout => false, :locals => { + :entry => entry, :current_user_id => current_user.id, :expand_comments => expand_comments } + end end post "/approve_commit" do @@ -537,6 +544,7 @@ def ensure_required_params(*required_params) :commits_with_closed_comments => closed_comments, :review_list_ids => review_list_order, :current_user_id => current_user.id, + :expand_comments => false, } end @@ -550,6 +558,7 @@ def ensure_required_params(*required_params) locals = { :review_list_ids => [params[:name]], :current_user_id => current_user.id, + :expand_comments => false, } case params[:name] when "uncompleted_reviews" diff --git a/models/commit.rb b/models/commit.rb index 4c0b8d52..e039ef3d 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -185,6 +185,24 @@ def self.paginate_dataset(dataset, page_by_cols, token, direction, page_size) [rows, token] end + def self.actionable_comments(commit, user_id) + grit_commit = MetaRepo.instance.grit_commit(commit.git_repo.name, commit[:sha]) + return nil unless grit_commit + if commit.author && commit.author.user_id == user_id + comments = Comment.filter(:commit_id => commit[:id]). + filter(:action_required => true, :comments__closed_at => nil). + where({ :comments__resolved_at => nil } | { :comments__user_id => user_id }).all + else + comments = Comment.filter(:commit_id => commit[:id]). + filter(:action_required => true, :comments__closed_at => nil). + where({ :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }).all + end + return nil if comments.empty? + entry = ReviewListEntry.new(grit_commit) + entry.comments = comments + entry + end + # Selects for the given user the commits with "actionable" comments, that is, comments that # are New and for user, or Resolved and from user. def self.commits_with_actionable_comments(user_id, token = nil, direction = "next", page_size = PAGE_SIZE) diff --git a/public/coffee/reviews.coffee b/public/coffee/reviews.coffee index 44453637..18de7e9b 100644 --- a/public/coffee/reviews.coffee +++ b/public/coffee/reviews.coffee @@ -27,7 +27,7 @@ window.Reviews = onExpandCommentsList: (e) -> target = $(e.currentTarget) - target.parents(".commentEntryRow").find(".overflowCommentRow").show() + target.parents(".commentEntryRow").find(".commentHidden").show() target.parents(".overflowButtonRow").hide() false @@ -43,19 +43,19 @@ window.Reviews = onCommentComplete: (e) -> target = $(e.currentTarget) commentId = target.data("commentId") + expanded = (target.parents(".commentEntryRow").find(".overflowCommentRow").css("display") != "none") $.ajax({ type: "post", url: "/close_comment", data: { comment_id: commentId + expand_comments: expanded } success: (html) => - table = target.parents(".innerCommitsList") - count = table.find(".commentRow").size() - target.parents(".commentRow").remove() - # if that was the last comment, then remove the commit line too - if count == 1 - table.parents(".commentEntryRow").remove() + if html.length == 0 + target.parents(".commentEntryRow").remove() + else + target.parents(".commentEntryRow").replaceWith(html) }) false diff --git a/public/css/saved_search.scss b/public/css/saved_search.scss index a7f550f4..e8d69791 100644 --- a/public/css/saved_search.scss +++ b/public/css/saved_search.scss @@ -203,7 +203,7 @@ } div.collapsedList { display: none; } .commitRow { background: #fcf8dc; } - .overflowCommentRow { display: none; } + .commentHidden { display: none; } .showOverflowComments { padding: 2px 4px; background: lightgrey; diff --git a/views/_commit_with_comments_entry.erb b/views/_commit_with_comments_entry.erb index 6574e50e..7e8a848f 100644 --- a/views/_commit_with_comments_entry.erb +++ b/views/_commit_with_comments_entry.erb @@ -3,6 +3,7 @@ View arguments: - entry: a ReviewListEntry - current_user_id: the current user id + - expand_comments: if true, then mark overflow comments as visible %> <% commit = entry.grit_commit %> <% comments = entry.comments %> @@ -41,8 +42,9 @@ <% end %> + <% overflow_class = expand_comments ? "overflowCommentRow" : "overflowCommentRow commentHidden" %> <% comments.each_with_index do |comment, index| %> - + x @@ -53,7 +55,7 @@ <% end %> - <% if comments.length >= 4 %> + <% if comments.length >= 4 && !expand_comments %> Show all <%= comments.length %> comments diff --git a/views/_commits_with_comments_list.erb b/views/_commits_with_comments_list.erb index 25b91684..53721b73 100644 --- a/views/_commits_with_comments_list.erb +++ b/views/_commits_with_comments_list.erb @@ -8,6 +8,7 @@ - review_list: a ReviewList containing an array of ReviewListEntry's - current_user_id: the current user id, passed to the next erb file - collapsed: true if list should be shown in the collapsed state (only header is displayed) + - expand_comments: if true, then mark overflow comments as visible, passed to the next erb file %> <% entries = review_list.entries %> @@ -36,7 +37,7 @@ <% entries.each do |entry| %> <%= erb :_commit_with_comments_entry, :layout => false, :locals => { :entry => entry, - :current_user_id => current_user_id } %> + :current_user_id => current_user_id, :expand_comments => expand_comments } %> <% end %>
    diff --git a/views/reviews.erb b/views/reviews.erb index c9692ccd..bb4790fa 100644 --- a/views/reviews.erb +++ b/views/reviews.erb @@ -25,6 +25,7 @@ :labelClass => "", :header => "Actionable comments", :review_list => commits_with_actionable_comments, + :expand_comments => expand_comments, :current_user_id => current_user_id } %> <% end %> @@ -58,6 +59,7 @@ :labelClass => "", :header => "Pending comments", :review_list => commits_with_pending_comments, + :expand_comments => expand_comments, :current_user_id => current_user_id } %> <% end %> @@ -69,6 +71,7 @@ :labelClass => "", :header => "Closed comments", :review_list => commits_with_closed_comments, + :expand_comments => expand_comments, :current_user_id => current_user_id } %> <% end %> From 8ad60ca7a43e3fe7175a61638d31ef62cb26516e Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sat, 9 Mar 2013 21:32:53 -0800 Subject: [PATCH 70/71] Refactor commits_with_actionable_comments() to use actionable_comments(). --- models/commit.rb | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/models/commit.rb b/models/commit.rb index e039ef3d..a7c4e183 100644 --- a/models/commit.rb +++ b/models/commit.rb @@ -186,9 +186,11 @@ def self.paginate_dataset(dataset, page_by_cols, token, direction, page_size) end def self.actionable_comments(commit, user_id) - grit_commit = MetaRepo.instance.grit_commit(commit.git_repo.name, commit[:sha]) + git_repo_name = commit[:git_repo_name] || commit.git_repo.name + grit_commit = MetaRepo.instance.grit_commit(git_repo_name, commit[:sha]) return nil unless grit_commit - if commit.author && commit.author.user_id == user_id + author_user_id = commit[:author_user_id] || (commit.author && commit.author.user_id) + if author_user_id == user_id comments = Comment.filter(:commit_id => commit[:id]). filter(:action_required => true, :comments__closed_at => nil). where({ :comments__resolved_at => nil } | { :comments__user_id => user_id }).all @@ -212,7 +214,8 @@ def self.commits_with_actionable_comments(user_id, token = nil, direction = "nex # and are not resolved (and not closed), or # 2b. Comments that this user made on some commit that are resolved (but not closed). token = ReviewList.make_token(0, 0, 0, false) if token.nil? - dataset = Commit.select(:commits__id, :git_repos__name, :sha, :authors__user_id___authors_user_id). + dataset = Commit.select(:commits__id, :git_repos__name___git_repo_name, :sha, + :authors__user_id___author_user_id). join(:comments, :commit_id => :id). join(:authors, :id => :commits__author_id). join(:git_repos, :id => :commits__git_repo_id). @@ -224,19 +227,8 @@ def self.commits_with_actionable_comments(user_id, token = nil, direction = "nex commits, token = paginate_dataset(dataset, [:commits__id], token, direction, page_size) entries = [] commits.each do |commit| - grit_commit = MetaRepo.instance.grit_commit(commit[:name], commit[:sha]) - next unless grit_commit - if commit[:authors_user_id] == user_id - comments = Comment.filter(:commit_id => commit[:id]). - filter(:action_required => true, :comments__closed_at => nil). - where({ :comments__resolved_at => nil } | { :comments__user_id => user_id }).all - else - comments = Comment.filter(:commit_id => commit[:id]). - filter(:action_required => true, :comments__closed_at => nil). - where({ :comments__user_id => user_id } & ~{ :comments__resolved_at => nil }).all - end - entry = ReviewListEntry.new(grit_commit) - entry.comments = comments + entry = actionable_comments(commit, user_id) + next if entry.nil? entries << entry end ReviewList.new(entries, token) From 2ae9c32a75067bc04b44c0f7ed949e1610c0ff68 Mon Sep 17 00:00:00 2001 From: Jack Veenstra Date: Sun, 10 Mar 2013 15:16:15 -0700 Subject: [PATCH 71/71] Group related comments within a review list by adding a border. --- public/css/saved_search.scss | 14 ++++++++++++++ views/_commit_with_comments_entry.erb | 15 +++++++-------- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/public/css/saved_search.scss b/public/css/saved_search.scss index e8d69791..e5665782 100644 --- a/public/css/saved_search.scss +++ b/public/css/saved_search.scss @@ -209,6 +209,20 @@ background: lightgrey; border-radius: 3px; } + .innerCommitsWithCommentsList { + margin-bottom: 10px; + td.leftBorderCell { + border-left: solid 1px; + padding-left: 5px; + } + td.topBorderCell { + border-top: solid 1px; + } + td.bottomBorderCell { + border-bottom: solid 1px; + padding-bottom: 5px; + } + } .centerText { text-align: center; } } diff --git a/views/_commit_with_comments_entry.erb b/views/_commit_with_comments_entry.erb index 7e8a848f..6af6f6ed 100644 --- a/views/_commit_with_comments_entry.erb +++ b/views/_commit_with_comments_entry.erb @@ -23,16 +23,16 @@ <%= commit.author.to_s[0..25] %> - +
    - - - - + + - - <% end %> -
    + <%= commit.id_abbrev %> + <%= CGI::escapeHTML(commit.short_message) %> <%= commit.date.to_pretty %><%= commit.repo_name %><%= commit.date.to_pretty %><%= commit.repo_name %> <% comment_count = db_commit.comment_count %> <% if comment_count > 0 %> @@ -45,7 +45,7 @@ <% overflow_class = expand_comments ? "overflowCommentRow" : "overflowCommentRow commentHidden" %> <% comments.each_with_index do |comment, index| %>
    + x @@ -57,11 +57,10 @@ <% end %> <% if comments.length >= 4 && !expand_comments %>
    + Show all <%= comments.length %> comments