-
Notifications
You must be signed in to change notification settings - Fork 3
Add review coverage model #601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # == 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 } | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| 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, | ||
| coverage_percentage: coverage_percentage | ||
| ) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def total_files | ||
| @total_files ||= github_client.files.count | ||
| end | ||
|
|
||
| def files_with_comments_count | ||
| @files_with_comments_count ||= | ||
| github_client.comments.map { |comment| { filename: comment[:path] } }.uniq.count | ||
| end | ||
|
|
||
| def coverage_percentage | ||
| (files_with_comments_count.to_f / total_files).round(2) | ||
| end | ||
|
|
||
| def github_client | ||
| @github_client ||= GithubClient::PullRequest.new(@pull_request) | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,10 +16,29 @@ def get | |
| def files | ||
| url = "repositories/#{repository.github_id}/pulls/#{pull_request.number}/files" | ||
| get_all_paginated_items(url, MAX_FILES_PER_PAGE) | ||
| rescue Faraday::Error => exception | ||
| handle_exception(exception) | ||
| [] | ||
| end | ||
|
|
||
| def comments | ||
| url = "repos/#{repository.full_name}/pulls/#{pull_request.number}/comments" | ||
| get_all_paginated_items(url, MAX_FILES_PER_PAGE) | ||
| rescue Faraday::Error => exception | ||
| handle_exception(exception) | ||
| [] | ||
| end | ||
|
|
||
| private | ||
|
|
||
| attr_reader :pull_request | ||
|
|
||
| def handle_exception(exception) | ||
| Honeybadger.notify(exception) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added Honeybadger because it is the tool being used, but I don't have access to it. I think in a future PR, we can move to Sentry or something similar. I added it anyway to keep track of this error notification. |
||
| Rails.logger.error( | ||
| "Failed to retrieve data for pull request #{pull_request.number} " \ | ||
| "from repository #{repository.full_name}: #{exception.message}" | ||
| ) | ||
| end | ||
| end | ||
| end | ||
| 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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aren't we losing all the old values?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| 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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| # == 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) } | ||
| coverage_percentage { (files_with_comments_count.to_f / total_files_changed).round(2) } | ||
| pull_request | ||
| end | ||
| end |
There was a problem hiding this comment.
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?