-
Notifications
You must be signed in to change notification settings - Fork 11
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-5006: fix Sentry issue #1979
CV2-5006: fix Sentry issue #1979
Conversation
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'll follow up in the ticket on why we can't just ignore the cases where the data is not present.
lib/check_statistics.rb
Outdated
@@ -95,7 +95,7 @@ def number_of_whatsapp_conversations(team_id, start_date, end_date, type = 'all' | |||
all = 0 | |||
user = 0 | |||
business = 0 | |||
data['conversation_analytics']['data'][0]['data_points'].each do |data_point| | |||
data.dig('conversation_analytics', 'data', 0, 'data_points')&.each do |data_point| |
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.
@caiosba I suggest to sent a notification in case data_points is nil
i.e
data_points = data.dig('conversation_analytics', 'data', 0, 'data_points')
send_notification if data_points.nil?
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.
OK! Let's add details to the ticket.
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.
@caiosba I see that the message that I should add will be close to the Sentry error so I keep raising error for nil data be removing &.each do |data_point|
and iterate data without &.
…hts-api-error-could-not-get-whats-app-conversations-statistics
@@ -95,7 +95,7 @@ def number_of_whatsapp_conversations(team_id, start_date, end_date, type = 'all' | |||
all = 0 | |||
user = 0 | |||
business = 0 | |||
data['conversation_analytics']['data'][0]['data_points'].each do |data_point| | |||
data.dig('conversation_analytics', 'data', 0, 'data_points').each do |data_point| |
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, if we make this change, what is going to trigger this line? -> https://github.com/meedan/check-api/blob/develop/lib/check_statistics.rb#L115. My suggestion would be to raise an exception if that array is empty but the workspace has TiplineMessage
's for the month.
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.
@caiosba this line https://github.com/meedan/check-api/blob/develop/lib/check_statistics.rb#L115 will trigger due to using each
for nil class so expected this error undefined method
each' for nil:NilClass`
I do not recommend to do extra queries so we can close this PR as keep things as it what do you think?
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, better to keep it as is, then.
Description
Fix sentry issue.
References: CV2-5006
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can verify the changes. Please describe whether or not you 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