CV2-2648 Refactor Tipline requests #1743

melsawy · 2023-11-27T04:05:46Z

Description

Convert smooch annotations to a new model TiplineRequest through the following steps:

Convert smooch fields to a new model TiplineRequest and create new fields from smooch_data (language, tipline_user_uid and platform) to make PG queries fast
Remove smooch fields migration
Store requests in a new model instead of smooch annotations
Fix ES for tipline requests
Fix cached fields related to smooch annotations
Fix team rules
Fix seed file
Fix check statistics

References: CV2-2648

How has this been tested?

Fix existing tests and make sure travis tests passed.
Run tipline integration locally

Things to pay attention to during code review

Please describe parts of the change that require extra attention during code review, for example:

File FFFF, line LL: This refactoring does this and this. Is it consistent with how it’s implemented elsewhere?
Etc.

Checklist

I have performed a self-review of my own code
I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
I have added regression tests, if the PR fixes a bug
I have added logging, exception reporting, and custom tracing with any additional information required for debugging
I considered secure coding practices when writing this code. Any security concerns are noted above.
I have commented my code in hard-to-understand areas, if any
I have made needed changes to the README
My changes generate no new warnings
If I added a third party module, I included a rationale for doing so and followed our current guidelines

caiosba

Great work Sawy! Getting there!

db/migrate/20231122054128_create_tipline_requests.rb

app/graph/mutations/tipline_message_mutations.rb

app/models/concerns/project_media_cached_fields.rb

app/models/concerns/smooch_messages.rb

caiosba · 2023-12-05T16:49:08Z

lib/check_statistics.rb

+      conditions = {
+        created_at: start_date..end_date,
+        team_id: team_id,
+        language: language,
+        platform: platform
+      }
+      conditions[:smooch_request_type] = type unless type.nil?
+      TiplineRequest.where(conditions)


Way cleaner!

lib/check_statistics.rb

caiosba · 2023-12-05T16:57:18Z

lib/check_statistics.rb

-            times << (f.created_at - f.annotation.created_at)
+          reports_received(team_id, platform, start_date, end_date, language).find_each do |tr|
+            # TODO: review with Caio
+            times << (tr.updated_at - tr.created_at)
          end
          median_response_time_in_seconds = times.size == 0 ? nil : times.sum.to_f / times.size
          statistics[:median_response_time] = median_response_time_in_seconds


The rest of the changes here look good, but please run the rake task to be sure.

Sure, I'll do

@melsawy , did you run the rake task?

caiosba · 2023-12-05T17:02:03Z

db/migrate/20231122054128_create_tipline_requests.rb

+      t.string :language, index: true
+      t.string :tipline_user_uid, index: true
+      t.string :platform, index: true
+      t.text :smooch_request_type, null: true


Request type can't be null.

app/models/tipline_request.rb

caiosba · 2024-01-03T19:32:16Z

lib/check_statistics.rb

@@ -16,7 +16,7 @@ def requests(team_id, platform, start_date, end_date, language, type = nil)
    def reports_received(team_id, platform, start_date, end_date, language)
      TiplineRequest
        .where(team_id: team_id, language: language, created_at: start_date..end_date, platform: platform)
-        .where('smooch_report_received_at IS NOT NULL')
+        .where.not(smooch_report_received_at: 0)


I think it can still be NULL, no? I think it's more robust to change this condition to:

.where('smooch_report_received_at > 0')

No as I set the default value to 0 in migration file so it'll be a value or 0

caiosba · 2024-01-03T19:32:42Z

lib/check_statistics.rb

-            times << (f.created_at - f.annotation.created_at)
+          reports_received(team_id, platform, start_date, end_date, language).find_each do |tr|
+            # TODO: review with Caio
+            times << (tr.updated_at - tr.created_at)
          end
          median_response_time_in_seconds = times.size == 0 ? nil : times.sum.to_f / times.size
          statistics[:median_response_time] = median_response_time_in_seconds


@melsawy , did you run the rake task?

caiosba · 2024-01-03T19:33:58Z

lib/tasks/migrate/20231122054128_migrate_tipline_requests.rake

@@ -0,0 +1,220 @@
+namespace :check do


Please document here (as a comment, first line of the file) how to call the rake tasks (expected args, etc.)

Already I added a line before rake task definition bundle exec rails check:migrate:migrate_tipline_requests

caiosba · 2024-01-03T19:36:33Z

test/models/bot/smooch_8_test.rb

Why this file was deleted? No more useful tests here?

Yes this file contains only one test and no more useful

caiosba · 2024-01-03T19:37:00Z

test/test_helper.rb

@@ -159,7 +159,8 @@ def before_all
  # This will run before any test

  def setup
-    [Account, Media, ProjectMedia, User, Source, Annotation, Team, TeamUser, Relationship, Project, TiplineResource].each{ |klass| klass.delete_all }
+    puts "[Starting test #{self.class.name}::#{self.method_name}]" # Output which test has started running


Please comment this line

caiosba · 2024-01-03T19:37:11Z

test/test_helper.rb

@@ -195,6 +196,7 @@ def setup
  # This will run after any test

  def teardown
+    puts "[Finishing test #{self.class.name}::#{self.method_name}]" # Output which test has finished running


Please comment this line

…com:meedan/check-api into refactor/CV2-2648-refactor-tipline-requests

melsawy · 2024-01-09T05:28:16Z

@caiosba I applied latest comments and asked @sonoransun for a recent live dump so I can verify tipline refactor rake task and statistic rake task

caiosba

Thanks @melsawy - please re-request my review once the other issues are fixed too (see testing sheet).

melsawy · 2024-01-23T16:07:34Z

@caiosba please review the latest changes and please verify rake tasks on your local machine

bundle exec rake check:data:statistics
bundle exec rails check:migrate:migrate_tipline_requests

caiosba

@melsawy , I just reviewed the latest changes to the PR and tested the migration and statistic rake tasks locally. All good except for one thing: the “average time for publishing” data point is not being calculated correctly (or maybe the data points involved were not migrated correctly?). More details in Jira.

caiosba

Sawy, it still doesn't work. I added more details to the Jira ticket.

…com:meedan/check-api into refactor/CV2-2648-refactor-tipline-requests

codeclimate · 2024-01-27T17:54:38Z

Code Climate has analyzed commit 69b6f16 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (100% is the threshold).

This pull request will bring the total coverage in the repository to 100.0% (0.0% change).

View more on Code Climate.

caiosba

Thanks @melsawy , the statistics rake task works for me now.

melsawy added 15 commits November 27, 2023 05:11

CV2-2648: add tipline request model

03b1e5e

CV2-2648: replace smooch_data queries with TiplineRequest

7fb3280

CV2-2648: fix cached fields

d20081c

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

fb2b0be

CV2-2648: remove unneeded migration

0c12a55

CV2-2648: fix tests 1/2

0349c1a

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

fd11aad

CV2-2648: fix syntex

0034747

CV2-2648: fix tests 2/2

40b2594

CV2-2648: rename methods and args

8471ea6

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

8d06298

CV2-2648: fix typo

87d6df2

CV2-2648: fix seed file and change TiplineRequest column types

2032739

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

f15e1d7

CV2-2648: fix check statistics

a3c4517

melsawy changed the title ~~Refactor/cv2 2648 refactor tipline requests~~ CV2-2648 Refactor Tipline requests Dec 4, 2023

melsawy added 6 commits December 4, 2023 16:44

CV2-2648: fix tests

5edda56

CV2-2648: add more tests

3649148

CV2-2648

3d2f03d

CV2-2648: test coverage

22c523b

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

281a434

CV2-2648: cleanup

c1be772

melsawy marked this pull request as ready for review December 5, 2023 06:18

melsawy requested review from caiosba and DGaffney as code owners December 5, 2023 06:18

caiosba requested changes Dec 5, 2023

View reviewed changes

melsawy added 4 commits December 10, 2023 18:52

CV2-2648: apply PR comments

fdc268d

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

d392352

CV2-2648: fix tests

8ec8d40

CV2-2648: fix tests

ebcdd07

caiosba added 2 commits December 27, 2023 21:04

Fixing tests

a4f0ce7

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

5b0d7c2

caiosba requested changes Jan 3, 2024

View reviewed changes

melsawy added 4 commits January 8, 2024 19:01

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

a99b61d

CV2-2648: apply PR comments

d683076

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

16f4ca2

Merge branch 'refactor/CV2-2648-refactor-tipline-requests' of github.…

90ad25b

…com:meedan/check-api into refactor/CV2-2648-refactor-tipline-requests

melsawy requested a review from caiosba January 9, 2024 05:19

caiosba reviewed Jan 10, 2024

View reviewed changes

melsawy added 2 commits January 18, 2024 07:32

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

3df445e

CV2-2648: allow rake task to accept args(slug and batch size)

26ed154

melsawy requested a review from caiosba January 23, 2024 16:05

caiosba approved these changes Jan 24, 2024

View reviewed changes

caiosba requested changes Jan 24, 2024

View reviewed changes

CV2-2648: Get reports received based on smooch_report_received_at field

c15b4b6

melsawy requested a review from caiosba January 24, 2024 09:20

Fixing a bug

ce239b6

caiosba requested changes Jan 24, 2024

View reviewed changes

melsawy added 5 commits January 25, 2024 16:50

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

45b779f

Merge branch 'refactor/CV2-2648-refactor-tipline-requests' of github.…

fb75157

…com:meedan/check-api into refactor/CV2-2648-refactor-tipline-requests

CV2-2648: fix average time for statistics data

7a9c11b

Merge branch 'develop' into refactor/CV2-2648-refactor-tipline-requests

39fc1ff

CV2-2648: remove unneeded file

69b6f16

melsawy requested a review from caiosba January 27, 2024 16:30

caiosba approved these changes Jan 27, 2024

View reviewed changes

melsawy merged commit f5debd5 into develop Jan 28, 2024
8 checks passed

melsawy deleted the refactor/CV2-2648-refactor-tipline-requests branch January 28, 2024 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CV2-2648 Refactor Tipline requests #1743

CV2-2648 Refactor Tipline requests #1743

melsawy commented Nov 27, 2023 •

edited

Loading

caiosba left a comment

caiosba Dec 5, 2023

caiosba Dec 5, 2023

melsawy Dec 6, 2023

caiosba Jan 3, 2024

caiosba Dec 5, 2023

caiosba Jan 3, 2024

melsawy Jan 4, 2024

caiosba Jan 3, 2024

caiosba Jan 3, 2024

melsawy Jan 4, 2024

caiosba Jan 3, 2024

melsawy Jan 4, 2024

caiosba Jan 3, 2024

melsawy Jan 4, 2024

caiosba Jan 3, 2024

melsawy Jan 4, 2024

melsawy commented Jan 9, 2024

caiosba left a comment

melsawy commented Jan 23, 2024

caiosba left a comment

caiosba left a comment

codeclimate bot commented Jan 27, 2024

caiosba left a comment

CV2-2648 Refactor Tipline requests #1743

CV2-2648 Refactor Tipline requests #1743

Conversation

melsawy commented Nov 27, 2023 • edited Loading

Description

How has this been tested?

Things to pay attention to during code review

Checklist

caiosba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melsawy commented Jan 9, 2024

caiosba left a comment

Choose a reason for hiding this comment

melsawy commented Jan 23, 2024

caiosba left a comment

Choose a reason for hiding this comment

caiosba left a comment

Choose a reason for hiding this comment

codeclimate bot commented Jan 27, 2024

caiosba left a comment

Choose a reason for hiding this comment

melsawy commented Nov 27, 2023 •

edited

Loading