Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions app/models/events/pull_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class PullRequest < ApplicationRecord
dependent: :destroy, inverse_of: :pull_request
has_many :events, as: :handleable, dependent: :destroy
has_one :merge_time, dependent: :destroy
has_one :review_coverage, dependent: :destroy

validates :state, inclusion: { in: states.keys }
validates :github_id,
Expand All @@ -65,13 +66,5 @@ class PullRequest < ApplicationRecord
:locked,
inclusion: { in: [true, false] }
validates :github_id, uniqueness: true, strict: PullRequests::GithubUniquenessError

after_validation :build_merge_time, on: :update, if: :merged_at_changed?

private

def build_merge_time
Builders::MergeTime.call(self)
end
end
end
3 changes: 2 additions & 1 deletion app/models/metric_definition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ class MetricDefinition < ApplicationRecord
defect_escape_rate: 'defect_escape_rate',
pull_request_size: 'pull_request_size',
development_cycle: 'development_cycle',
planned_to_done: 'planned_to_done' }
planned_to_done: 'planned_to_done',
review_coverage: 'review_coverage' }

validates :code, uniqueness: true
validates :name, presence: true, uniqueness: true
Expand Down
40 changes: 40 additions & 0 deletions app/models/review_coverage.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# == Schema Information
#
# Table name: review_coverages
#
# id :bigint not null, primary key
# coverage_percentage :decimal(, ) not null
# deleted_at :datetime
# files_with_comments_count :integer not null
# total_files_changed :integer not null
# created_at :datetime not null
# updated_at :datetime not null
# pull_request_id :bigint not null
#
# Indexes
#
# index_review_coverages_on_pull_request_id (pull_request_id)
#
# Foreign Keys
#
# fk_rails_... (pull_request_id => events_pull_requests.id)
#

class ReviewCoverage < ApplicationRecord
acts_as_paranoid

belongs_to :pull_request, class_name: 'Events::PullRequest'

validates :total_files_changed, :files_with_comments_count, presence: true
validates :pull_request_id, uniqueness: true
validates :total_files_changed, :files_with_comments_count,
numericality: { greater_than_or_equal_to: 0 }

before_save :calculate_coverage_percentage
Copy link
Contributor

Choose a reason for hiding this comment

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

it's necessary to be callback or can be calculated when creating/updating the object? Just asking because I think is a good practice to avoid this type of callbacks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved! 💪


private

def calculate_coverage_percentage
self.coverage_percentage = (files_with_comments_count.to_f / total_files_changed).round(2)
end
end
6 changes: 5 additions & 1 deletion app/services/action_handlers/pull_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ def open
end

def merged
@entity.update!(merged_at: Time.current)
ActiveRecord::Base.transaction do
@entity.update!(merged_at: Time.current)
Builders::MergeTime.call(@entity)
Builders::ReviewCoverage.call(@entity)
end
end

def closed
Expand Down
29 changes: 29 additions & 0 deletions app/services/builders/review_coverage.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
module Builders
class ReviewCoverage < BaseService
def initialize(pull_request)
@pull_request = pull_request
end

def call
::ReviewCoverage.create!(
pull_request: @pull_request,
total_files_changed: total_files,
files_with_comments_count: files_with_comments_count
)
end

private

def total_files
github_client.files.count
end

def files_with_comments_count
github_client.comments.map { |comment| { filename: comment[:path] } }.uniq.count
end

def github_client
@github_client ||= GithubClient::PullRequest.new(@pull_request)
end
end
end
5 changes: 5 additions & 0 deletions app/services/github_client/pull_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ def files
get_all_paginated_items(url, MAX_FILES_PER_PAGE)
end

def comments
url = "repos/#{repository.full_name}/pulls/#{pull_request.number}/comments"
get_all_paginated_items(url, MAX_FILES_PER_PAGE)
Copy link
Contributor

Choose a reason for hiding this comment

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

mm should we handle or log possible errors here?

end

private

attr_reader :pull_request
Expand Down
13 changes: 13 additions & 0 deletions db/migrate/20250429002929_create_review_coverages.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class CreateReviewCoverages < ActiveRecord::Migration[7.1]
def change
create_table :review_coverages do |t|
t.references :pull_request, null: false, foreign_key: { to_table: :events_pull_requests }
t.integer :total_files_changed, null: false
t.integer :files_with_comments_count, null: false
t.decimal :coverage_percentage, null: false
t.datetime :deleted_at

t.timestamps
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
class AddReviewCoverageToMetricNameType < ActiveRecord::Migration[7.1]
disable_ddl_transaction!

def up
execute <<-SQL
ALTER TYPE metric_name ADD VALUE 'review_coverage';
SQL
end

def down
execute <<-SQL
ALTER TYPE metric_name RENAME TO metric_name_old;
CREATE TYPE metric_name AS ENUM ('review_turnaround', 'blog_visits', 'merge_time', 'blog_post_count', 'open_source_visits', 'defect_escape_rate', 'pull_request_size', 'development_cycle');
ALTER TABLE metrics ALTER COLUMN name TYPE metric_name USING name::text::metric_name;
DROP TYPE metric_name_old;
Copy link
Member

Choose a reason for hiding this comment

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

aren't we losing all the old values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the previous line is transferring the values.

SQL
end
end
Comment on lines +1 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks very strange to me. How this enum defined ? is not enough to add the new enum options in the model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This enum is defined as a type at a database level, it's not an integer, and that's why I need to add the value with a migration.

1 change: 1 addition & 0 deletions db/seeds.rb
Original file line number Diff line number Diff line change
Expand Up @@ -151,4 +151,5 @@
MetricDefinition.create!(code: :pull_request_size, explanation: 'Measures the lines of code added by each PR. The smaller the PR, the easier it is to review, which speeds up the review process..', name: 'PR Size')
MetricDefinition.create!(code: :development_cycle, explanation: 'It measures how much time the team spends working on a task.', name: 'Development Cycle')
MetricDefinition.create!(code: :planned_to_done, explanation: 'The planned-to-done ratio measures what percentage of the tasks you set out for your team were completed satisfactorily.', name: 'Planned to Done Ratio')
MetricDefinition.create!(code: :review_coverage, explanation: 'Measures the percentage of files in a pull request that have received review comments.', name: 'Review Coverage')
end
69 changes: 68 additions & 1 deletion db/structure.sql
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ CREATE TYPE public.metric_name AS ENUM (
'defect_escape_rate',
'pull_request_size',
'development_cycle',
'planned_to_done'
'planned_to_done',
'review_coverage'
);


Expand Down Expand Up @@ -1294,6 +1295,41 @@ CREATE SEQUENCE public.repositories_id_seq
ALTER SEQUENCE public.repositories_id_seq OWNED BY public.repositories.id;


--
-- Name: review_coverages; Type: TABLE; Schema: public; Owner: -
--

CREATE TABLE public.review_coverages (
id bigint NOT NULL,
pull_request_id bigint NOT NULL,
total_files_changed integer NOT NULL,
files_with_comments_count integer NOT NULL,
coverage_percentage numeric NOT NULL,
deleted_at timestamp(6) without time zone,
created_at timestamp(6) without time zone NOT NULL,
updated_at timestamp(6) without time zone NOT NULL
);


--
-- Name: review_coverages_id_seq; Type: SEQUENCE; Schema: public; Owner: -
--

CREATE SEQUENCE public.review_coverages_id_seq
START WITH 1
INCREMENT BY 1
NO MINVALUE
NO MAXVALUE
CACHE 1;


--
-- Name: review_coverages_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: -
--

ALTER SEQUENCE public.review_coverages_id_seq OWNED BY public.review_coverages.id;


--
-- Name: review_requests; Type: TABLE; Schema: public; Owner: -
--
Expand Down Expand Up @@ -1711,6 +1747,13 @@ ALTER TABLE ONLY public.products ALTER COLUMN id SET DEFAULT nextval('public.pro
ALTER TABLE ONLY public.repositories ALTER COLUMN id SET DEFAULT nextval('public.repositories_id_seq'::regclass);


--
-- Name: review_coverages id; Type: DEFAULT; Schema: public; Owner: -
--

ALTER TABLE ONLY public.review_coverages ALTER COLUMN id SET DEFAULT nextval('public.review_coverages_id_seq'::regclass);


--
-- Name: review_requests id; Type: DEFAULT; Schema: public; Owner: -
--
Expand Down Expand Up @@ -2001,6 +2044,14 @@ ALTER TABLE ONLY public.repositories
ADD CONSTRAINT repositories_pkey PRIMARY KEY (id);


--
-- Name: review_coverages review_coverages_pkey; Type: CONSTRAINT; Schema: public; Owner: -
--

ALTER TABLE ONLY public.review_coverages
ADD CONSTRAINT review_coverages_pkey PRIMARY KEY (id);


--
-- Name: review_requests review_requests_pkey; Type: CONSTRAINT; Schema: public; Owner: -
--
Expand Down Expand Up @@ -2463,6 +2514,13 @@ CREATE INDEX index_repositories_on_language_id ON public.repositories USING btre
CREATE INDEX index_repositories_on_product_id ON public.repositories USING btree (product_id);


--
-- Name: index_review_coverages_on_pull_request_id; Type: INDEX; Schema: public; Owner: -
--

CREATE INDEX index_review_coverages_on_pull_request_id ON public.review_coverages USING btree (pull_request_id);


--
-- Name: index_review_requests_on_owner_id; Type: INDEX; Schema: public; Owner: -
--
Expand Down Expand Up @@ -2660,6 +2718,13 @@ ALTER TABLE ONLY public.events_pushes
ADD CONSTRAINT fk_rails_3f633d82fd FOREIGN KEY (repository_id) REFERENCES public.repositories(id);


--
-- Name: review_coverages fk_rails_40af85f049; Type: FK CONSTRAINT; Schema: public; Owner: -
--

ALTER TABLE ONLY public.review_coverages
ADD CONSTRAINT fk_rails_40af85f049 FOREIGN KEY (pull_request_id) REFERENCES public.events_pull_requests(id);


--
-- Name: events_pushes fk_rails_4767e99b87; Type: FK CONSTRAINT; Schema: public; Owner: -
Expand Down Expand Up @@ -2936,6 +3001,8 @@ INSERT INTO "schema_migrations" (version) VALUES
('20250430195700'),
('20250430195657'),
('20250430195652'),
('20250430151601'),
('20250429002929'),
('20240829142623'),
('20240627133952'),
('20221228121949'),
Expand Down
23 changes: 23 additions & 0 deletions lib/tasks/review_coverage_backfill.rake
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
namespace :review_coverage do
desc 'Backfill review coverage for merged pull requests'
task backfill: :environment do
puts 'Starting review coverage backfill...'

pull_requests = Events::PullRequest
.where.not(merged_at: nil)
.where.not(id: ReviewCoverage.select(:pull_request_id))
.where(created_at: Time.current.beginning_of_year..Time.current)

total = pull_requests.count
puts "Found #{total} pull requests to process"

pull_requests.find_each do |pull_request|
Builders::ReviewCoverage.call(pull_request)
rescue StandardError => exception
puts "Error processing PR ##{pull_request.number}
in repository #{pull_request.repository.id}: #{exception.message}"
end

puts 'Review coverage backfill completed!'
end
end
21 changes: 21 additions & 0 deletions spec/factories/events/pull_requests.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,26 @@
repository

association :owner, factory: :user

trait :merged do
state { 'closed' }
merged_at { Time.current }

transient do
merge_time { (merged_at - opened_at).to_i }
end

after(:create) do |pull_request, evaluator|
create(
:merge_time,
pull_request: pull_request,
value: evaluator.merge_time
)
create(
:review_coverage,
pull_request: pull_request
)
end
Comment on lines +65 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be created manually in each test that requires it. Maybe using a trait isn’t too bad, as long as we don’t overuse it, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that this logic is used in many tests, I thought it was a good idea to have the trait to avoid repeating all the logic. Also, it's a simple logic and not a complex trait. Do you think it's ok in this case?

end
end
end
29 changes: 29 additions & 0 deletions spec/factories/review_coverages.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# == Schema Information
#
# Table name: review_coverages
#
# id :bigint not null, primary key
# coverage_percentage :decimal(, ) not null
# deleted_at :datetime
# files_with_comments_count :integer not null
# total_files_changed :integer not null
# created_at :datetime not null
# updated_at :datetime not null
# pull_request_id :bigint not null
#
# Indexes
#
# index_review_coverages_on_pull_request_id (pull_request_id)
#
# Foreign Keys
#
# fk_rails_... (pull_request_id => events_pull_requests.id)
#

FactoryBot.define do
factory :review_coverage do
total_files_changed { Faker::Number.between(from: 1, to: 100) }
files_with_comments_count { Faker::Number.between(from: 0, to: total_files_changed) }
pull_request
end
end
20 changes: 0 additions & 20 deletions spec/models/events/pull_request_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,24 +77,4 @@
end
end
end

describe 'callbacks' do
context 'when a pull request is merged' do
before { travel_to(Time.zone.today.beginning_of_day) }
let!(:pull_request) { create(:pull_request, opened_at: Time.current) }

it 'creates a merge time with the correct values' do
pull_request.update!(merged_at: Time.current + 1.hour)
merge_time = MergeTime.last
expect(merge_time[:value].seconds).to eq(1.hour)
expect(merge_time[:pull_request_id]).to eq(pull_request.id)
end

it 'creates a merge time' do
expect {
pull_request.update!(merged_at: Time.current)
}.to change { MergeTime.count }.from(0).to(1)
end
end
end
end
Loading
Loading