Remove metrics definition from services #126
Conversation
… fixes the respective tests
hvilloria
requested review from
hdamico,
horacio,
santiagovidal and
santib
as code owners
app/jobs/process_metrics_job.rb
Outdated
| @@ -7,6 +7,6 @@ class ProcessMetricsJob < ApplicationJob | |||
| ## | |||
| # Delegates the processing to the MetricsDefinitionProcessor service. | |||
| def perform | |||
| Processors::MetricsDefinition.call | |||
| Processors::ReviewTurnaround.call | |||
There was a problem hiding this comment.
Shouldn't we change the file name also? For example process_review_turnaround_job
| BATCH_SIZE = 500 | ||
|
|
||
| def call | ||
| process |
There was a problem hiding this comment.
Why don't we have process body here?
| ENTITIES = ['UserProject'].freeze | ||
|
|
||
| def call | ||
| process |
There was a problem hiding this comment.
Same comment posted in app/services/metrics/review_turnaround/per_user_project.rb. Is this some convention agreed upon?
There was a problem hiding this comment.
Is a way to encapsulate the process method
There was a problem hiding this comment.
exactly @horacio, others processors and have it like that so i wanted to keep the consistency about how are made the services. as you commented it is like a convention
|
|
| end | ||
|
|
||
| def create_metric(entity, turnaround) | ||
| Metric.create!(ownable: entity, value: turnaround, name: :review_turnaround) |
There was a problem hiding this comment.
Shouldn't this have the timestamp?
There was a problem hiding this comment.
Also, what if that record exists already? Is it possible?
There was a problem hiding this comment.
-
what would be the timestamp for?
-
i think it is not possible because we pretend to process every events at some point during the day
There was a problem hiding this comment.
This metrics tells me which entity it applies to, the value, the name of the metric, but I have no idea what time period it applies to.
What does this PR do?
since the new idea is to generate metrics each day, some models and services like time intervals and metrics definitions were removed. Review turnaround flow has changed so the test use cases were changed too.
The main purpose is to process metrics at the end of every day, in what way we will be able to process week metrics, monthly metrics, etc. from these daily metrics.
issue: #120