-
Notifications
You must be signed in to change notification settings - Fork 14
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
CV2-5138: Data Dashboard: Write and analyze queries #2033
CV2-5138: Data Dashboard: Write and analyze queries #2033
Conversation
lib/check_data_points.rb
Outdated
smooch_request_type: SEARCH_RESULT_TYPES, | ||
created_at: start_date..end_date, | ||
).count | ||
c2 = TiplineRequest |
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.
Suggestion: For clarity, please rename this variable and/or add a comment that explains that "this is the number of articles sent as reports".
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.
Done
lib/check_data_points.rb
Outdated
created_at: start_date..end_date, | ||
).count | ||
c2 = TiplineRequest | ||
.where(smooch_request_type: ["default_requests", "timeout_requests"]) |
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.
@melsawy , I was thinking more about it, I think that we don't need this condition here. Because for a situation that a search result was returned but then an updated report was sent, I think we still want to count that.
@brianfleming , if you can confirm, please... imagine this situation:
- User sends a query
- The query returns a search result - this increments the statistics of "articles sent" by 1
- That same report is updated later on, and the user receives an update report - should this also increment "articles sent" by 1?
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.
yes @caiosba I agree. It matches with another discussion we had on analytics counting. starting with counting all the things now is a good idea. later if we need to get more specific we can
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.
Done
lib/check_data_points.rb
Outdated
def elastic_search_top_items(team_id, start_date, end_date, limit, with_tags = false) | ||
data = {} | ||
query = { | ||
range: {'created_at': { start_time: start_date, end_time: end_date } }, | ||
demand: { min: 1 }, | ||
sort: 'demand', | ||
eslimit: limit | ||
} | ||
query[:tags_as_sentence] = { min: 1 } if with_tags | ||
result = CheckSearch.new(query.to_json, nil, team_id) | ||
result.medias.each{|pm| data[pm.id] = pm.demand } | ||
data | ||
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.
Nice!
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.
Thanks Sawy, looks great so far!
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.
Looks good to me, thanks Sawy! Just one thing: before merging, please re-generate the schema.rb
file according to latest develop
... right now it has more stuff than it needs.
Done |
Description
Add a class for data points
References: CV2-5138
How has this been tested?
Implemented automated tests.
Things to pay attention to during code review
Please describe parts of the change that require extra attention during code review, for example:
Checklist