Add review coverage model#601
Conversation
gonzaM
requested review from
brunoparma88 and
lauperalti
and removed request for
a team
gonzaM
force-pushed
the
add_review_coverage_model
branch
from
0845656 to
fa09744
Compare
gonzaM
requested review from
rodrieiz and
santib
and removed request for
brunoparma88 and
lauperalti
app/models/review_coverage.rb
Outdated
| validates :total_files_changed, :files_with_comments_count, | ||
| numericality: { greater_than_or_equal_to: 0 } | ||
|
|
||
| before_save :calculate_coverage_percentage |
There was a problem hiding this comment.
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
|
|
||
| def comments | ||
| url = "repos/#{repository.full_name}/pulls/#{pull_request.number}/comments" | ||
| get_all_paginated_items(url, MAX_FILES_PER_PAGE) |
There was a problem hiding this comment.
mm should we handle or log possible errors here?
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
| require 'rails_helper' | ||
| require 'rake' | ||
|
|
||
| # rubocop:disable RSpec/DescribeClass |
There was a problem hiding this comment.
I way to find to avoid this is the task only calls another file review_coverage.rb
that have all the logic. feel free to ignore
| 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; | ||
| SQL | ||
| end | ||
| end |
There was a problem hiding this comment.
this looks very strange to me. How this enum defined ? is not enough to add the new enum options in the model?
There was a problem hiding this comment.
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.
| 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; |
There was a problem hiding this comment.
aren't we losing all the old values?
There was a problem hiding this comment.
I understand the previous line is transferring the values.
| attr_reader :pull_request | ||
|
|
||
| def handle_exception(exception) | ||
| Honeybadger.notify(exception) |
There was a problem hiding this comment.
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.
3 participants
What does this PR do?
This PR introduces a new code review metric that measures the percentage of files in a pull request that have received comments during the review process.
Changes