5441 – Change how we create imported fact-checks when an item already exists #2079

vasconsaurus · 2024-10-11T16:38:17Z

Description

Wanted Behavior

If the item has no fact-check: Append the fact-check to the existing ProjectMedia
If the item already has a fact-check in a different language: Create a fact-check associated with a blank media

Context

Importing a fact-check with a link original claim will fail when a project media with that link media already exists.

When importing, we create a project media to create the fact-check. If that project media has an original claim, we create a media as well. If the original claim is a link, we try to create a link media.

Since duplicate link medias are not permitted, if a link media with this link already exists, trying to create this new media from the same link will fail. Then creating the project media and fact-check will fail as well.

The issue with this

We will have the project media with the link media, but will fail to create the fact-check for it, even though the partner has one. If the partner has a fact-check, we want to be able to attach it to the correct existing project media.

A related issue

Sometimes a partner might have a fact-check for a project media, but if they have many languages, they might want to have more than one fact-check associated with that project media. Since this is currently not possible: if they have a fact-check in a different language, we want to create the fact-check associated to a blank media, instead of the link media.

References: CV2-5441

How has this been tested?

rails test test/controllers/graphql_controller_12_test.rb:735
rails test test/controllers/graphql_controller_12_test.rb:795

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

vasconsaurus · 2024-10-11T16:42:13Z

@caiosba when you have some time, could you please take a look into the tests?

rails test test/controllers/graphql_controller_12_test.rb:735
rails test test/controllers/graphql_controller_12_test.rb:792

test/controllers/graphql_controller_12_test.rb

caiosba · 2024-10-11T16:45:23Z

test/controllers/graphql_controller_12_test.rb

+      }
+    GRAPHQL
+
+    post :create, params: { query: query, team: t.slug }


I suggest we put a assert_no_difference 'ProjectMedia.count' and a assert_difference 'FactCheck.count' around this line.

caiosba · 2024-10-11T16:47:49Z

@caiosba when you have some time, could you please take a look into the tests?
rails test test/controllers/graphql_controller_12_test.rb:735
rails test test/controllers/graphql_controller_12_test.rb:792

Great start @vasconsaurus , that's exactly what we intended - I added a couple of suggestions to only one of the tests but they actually apply to both. It's just a couple of assert's to be sure we're just creating the data we expect.

vasconsaurus · 2024-10-16T11:52:07Z

@caiosba I'm going to start taking a look at the code today. Are there any specific steps you think I should take?

@jayjay-w Is there anything on the original claim side I should take into consideration? Do you have any suggestions on how to tackle this?

caiosba · 2024-10-16T12:11:34Z

@vasconsaurus my suggestion to start is to rescue the specific exception only for the ProjectMedia use case, and then call a method (defined in the model) that is going to handle the two special cases we outlined in the ticket.

Idea: Wrap the save call in a block that can rescue and handle these two cases.

First, try to do it here, which is specific for the create operation
If it doesn't work, do it here but make sure it applies only for the create operation

# FIXME: Refactor this as an around_save that GraphQL types can define themselves
begin
  self.safe_save(obj, attrs, parents_mapping.keys) # OR if idea (2) above: obj.save_with_version!
rescue StandardError => e # I know it's not StandardError, let's rescue the exception we currently get... we can replace the current exception class by a specific one if it makes things easier
  if obj.is_a?(ProjectMedia) && !obj.set_fact_check.blank? && !obj.set_original_claim.blank?
    obj = obj.handle_fact_check_for_existing_claim # We can then define this method in the ProjectMedia model and implement the logic for the two cases we listed in the ticket - it will be much easier to unit-test too
  else
    raise e
  end
end

Let me know if it makes sense! :)

But a question before you proceed: Were you able to reproduce in the two automated tests the same error we get from Zapier? Would be good to document it (e.g., the test run and the output) in the ticket.

jayjay-w · 2024-10-17T10:30:49Z

@vasconsaurus I do not have any special considerations that we need to take regarding original_claim, so Caio's rescue suggestion will be sufficient, in my opinion.

vasconsaurus · 2024-10-17T11:41:48Z

@caiosba I think I'll be able to get back to this one today, and will look into:

But a question before you proceed: Were you able to reproduce in the two automated tests the same error we get from Zapier? Would be good to document it (e.g., the test run and the output) in the ticket.

vasconsaurus · 2024-10-18T12:30:12Z

But a question before you proceed: Were you able to reproduce in the two automated tests the same error we get from Zapier? Would be good to document it (e.g., the test run and the output) in the ticket.

@caiosba it seems we do get the exact same error 🎉

From Zapier logs:

{
  "errors": [
    {
      "message": "This item already exists",
      "code": 9,
      "data": { ... }
    }
  ]
}

From our tests

"errors":[{"message":"This item already exists", "code":9, "data":{...}}]

caiosba · 2024-10-18T13:19:50Z

But a question before you proceed: Were you able to reproduce in the two automated tests the same error we get from Zapier? Would be good to document it (e.g., the test run and the output) in the ticket.

@caiosba it seems we do get the exact same error 🎉

From Zapier logs:
{
  "errors": [
    {
      "message": "This item already exists",
      "code": 9,
      "data": { ... }
    }
  ]
}
From our tests
"errors":[{"message":"This item already exists", "code":9, "data":{...}}]

That's perfect, Manu! 👌

vasconsaurus · 2024-10-22T18:43:18Z

@caiosba @jayjay-w
I realized I was only looking at the original claim as a link, and I noticed that it behaves differently when it's a link vs when it's another type. When it is a link if it is the same link the media creation fails. But if it is the same image file or quote it creates a new media. Is this the expected behavior?

I added new tests here

And ran manual tests for quote and image

caiosba · 2024-10-22T20:26:43Z

@caiosba @jayjay-w I realized I was only looking at the original claim as a link, and I noticed that it behaves differently when it's a link vs when it's another type. When it is a link if it is the same link the media creation fails. But if it is the same image file or quote it creates a new media. Is this the expected behavior?

I added new tests here

And ran manual tests for quote and image

@vasconsaurus yes, that's expected. We have a filter for existing items for media submissions that happen through tiplines:

I suggest you ignore it for now, and we can work on that in a separate, follow-up ticket. I added that to the ticket so we don't forget about it - let's revisit at the end.

caiosba · 2024-10-24T12:23:59Z

app/graph/mutations/graphql_crud_operations.rb

-    self.safe_save(obj, attrs, parents_mapping.keys)
+    begin
+      self.safe_save(obj, attrs, parents_mapping.keys)
+    rescue StandardError => e


Please remember to replace this by a more specific exception.

caiosba · 2024-10-24T12:24:14Z

app/models/project_media.rb

@@ -430,6 +430,22 @@ def apply_rules_and_actions_on_update
    self.team.apply_rules_and_actions(self, rule_ids)
  end

+  def handle_fact_check_for_existing_claim
+    # find the project media with the original claim media
+    pm = ProjectMedia.find_by(media_id: self.media_id)


Please filter by the team_id as well.

caiosba · 2024-10-24T12:30:48Z

app/graph/mutations/graphql_crud_operations.rb

+      self.safe_save(obj, attrs, parents_mapping.keys)
+    rescue StandardError => e
+      if obj.is_a?(ProjectMedia) && obj.set_fact_check.present? && obj.set_original_claim.present?
+        obj = obj.handle_fact_check_for_existing_claim


As per our conversation, let's turn this method into a class method:

obj = ProjectMedia.handle_fact_check_for_existing_claim(obj)

caiosba · 2024-10-24T12:31:09Z

app/models/project_media.rb

@@ -430,6 +430,22 @@ def apply_rules_and_actions_on_update
    self.team.apply_rules_and_actions(self, rule_ids)
  end

+  def handle_fact_check_for_existing_claim


As per comment above, turn this into a class method.

caiosba · 2024-10-24T12:34:02Z

app/models/project_media.rb

+        self.set_original_claim = nil
+        self.media_id = m.id


Once this becomes a class method, we can create a new instance here:

pm = obj.clone # If it doesn't work as expected, replace this by ProjectMedia.new pm.set_original_claim = nil pm.media_id = m.id pm.save!

app/models/project_media.rb

vasconsaurus · 2024-10-25T11:10:02Z

@caiosba I have questions! I think I might be missing something

Our first test (here) is currently passing. But I realized this line returns the fact check and not the project media. If I return the project media I get an error. That seems to be the same I'm getting for the second test, so moving on to that...
The second test, I tried saving the project media but I get the error below:

Failure:
GraphqlController12Test#test_should_create_a_FactCheck_with_a_Blank_ProjectMedia,_if_ProjectMedia_already_exists_and_has_a_FactCheck_in_a_different_language [/app/test/controllers/graphql_controller_12_test.rb:847]:
Expected response to be a <2XX: success>, but was a <400: Bad Request>
Response body: {"errors":[{"message":"          Failed to implement CreateProjectMedia.project_media, tried:\n\n          - `#\u003cClass:0x0000aaaae52f44d8\u003e#project_media`, which did not exist\n          - `TrueClass#project_media`, which did not exist\n          - Looking up hash key `:project_media` or `\"project_media\"` on `true`, but it wasn't a Hash\n\n          To implement this field, define one of the methods above (and check for typos)\n","code":5,"data":{}}]}

I'm sure I'm missing a lot of things about how this part of the code works. What should I be returning from the rescue? What am I missing here?

And one last question:
I tried testing using the models directly instead of going through the graphql test (since it can be slow). But I couldn't get it to work. I think because it doesn't really go through the graphql controller and that is where we adding the begin/rescue. Is that correct? And if it is, why do we only want to add this to graphql layer?

caiosba · 2024-10-25T11:46:22Z

@vasconsaurus hummm... please push what you have so I can take a look :)

I tried testing using the models directly instead of going through the graphql test (since it can be slow). But I couldn't get it to work.

For that, you should just test the handle_fact_check_for_existing_claim method directly.

so we are able to send an url

should append FactCheck to ProjectMedia should create a FactCheck with a Blank ProjectMedia

This shouldn't be StandardError, but will keep it for now.

it fails because of those two assertions, otherwise it passes looks like a syntax error, will look into it Monday

If the item already has a fact-check in a different language Currently broken

add unit tests to reflect current behavior

for this to work I had to change create_claim_description_and_fact_check to be public

The changes I added to sample_data had come side effects to other tests. So I'm rolling that back, and updated the two graphql tests to not need those changes anymore.

The error can be slightly different, parsing the way we were before doesn't work for every error. This caused test test/controllers/graphql_controller_12_test.rb:69, GraphqlController12Test#test_should_not_create_feed_invitation, to break.

vasconsaurus · 2024-10-29T18:16:55Z

@caiosba I finished the changes we talked about, could you take a look and see if this is ready for review or if there is something missing?

caiosba

Looks great @vasconsaurus , glad that the approach we discussed has worked as expected! I left a few comments, but the approach is correct. Did the tests pass?

caiosba · 2024-10-29T18:48:44Z

app/graph/mutations/graphql_crud_operations.rb

+    begin
+      obj.save_with_version!
+    rescue RuntimeError => e
+      if e.message.include?("This item already exists") &&


Request: @vasconsaurus , this is not a good way to check for the error because of two reasons:

(1) If I18n.locale is not English, then this error message will be localized, so won't match this condition (for example, for Portuguese it will be "Este item já existe").

(2) The copy for the error message may change in the future.

I suggest you check for the exception class or some other non-localized information.

@caiosba, questions:

The error we get is a RuntimeError, so it's not super specific. It does send the lapis code in the message, so I changed it to look for that. (here)

However, would it be worth for us to create a custom error? We get the error from here:

check-api/app/models/concerns/project_association.rb

Lines 139 to 155 in cb97946

def is_unique

obj_name = 'media'

obj = ProjectMedia.where(team_id: self.team_id, media_id: self.media_id).last

unless obj.nil?

error = {

message: I18n.t("#{obj_name}_exists", team_id: obj.team_id, id: obj.id),

code: LapisConstants::ErrorCodes::const_get('DUPLICATED'),

data: {

team_id: obj.team_id,

type: obj_name,

id: obj.id,

url: obj.full_url

}

}

raise error.to_json

end

end

@vasconsaurus , I don't think we need a specific code, I think that DUPLICATED is good enough for this case. But please use the variable LapisConstants::ErrorCodes::const_get('DUPLICATED') instead of the hard-coded value "9".

app/models/concerns/project_media_creators.rb

caiosba · 2024-10-29T18:53:26Z

app/models/project_media.rb

@@ -430,6 +430,31 @@ def apply_rules_and_actions_on_update
    self.team.apply_rules_and_actions(self, rule_ids)
  end

+  def self.handle_fact_check_for_existing_claim(existing_pm,new_pm)
+    if existing_pm.fact_check.blank?
+      existing_pm.appended_fact_check_from(new_pm)


Suggestion: Since this method takes an action, I suggest to name it with the verb name in imperative form, so, append_fact_check_from.

caiosba · 2024-10-29T18:53:39Z

app/models/project_media.rb

+      existing_pm.appended_fact_check_from(new_pm)
+    elsif existing_pm.fact_check.present?
+      if existing_pm.fact_check.language != new_pm.set_fact_check['language']
+        new_pm.replaced_with_blank_media


Suggestion: Since this method takes an action, I suggest to name it with the verb name in imperative form, so, replace_with_blank_media.

vasconsaurus · 2024-10-30T11:10:52Z

@caiosba

Did the tests pass?

Yes 🎉

Since this method takes an action, I suggest to name it with the verb name in imperative form

(1) If I18n.locale is not English, then this error message will be localized, so won't match this condition (for example, for Portuguese it will be "Este item já existe"). (2) The copy for the error message may change in the future.

…ED')

caiosba

Great work @vasconsaurus ! Thanks for addressing the comments from the previous reviews. I added a couple of suggestions mostly for clarity.

caiosba · 2024-10-30T23:18:21Z

app/graph/mutations/graphql_crud_operations.rb

+    begin
+      obj.save_with_version!
+    rescue RuntimeError => e
+      if e.message.include?("\"code\":#{LapisConstants::ErrorCodes::const_get('DUPLICATED')}") &&


You could also do a JSON.parse on the message and check for the code, but this is fine too.

I tried this before, but we ran into an issue with test GraphqlController12Test#test_should_not_create_feed_invitation / rails test test/controllers/graphql_controller_12_test.rb:69.

[11, 20] in /app/app/graph/mutations/graphql_crud_operations.rb 11: begin 12: obj.save_with_version! 13: rescue RuntimeError => e 14: require 'byebug' 15: byebug => 16: if e.message.include?("\"code\":#{LapisConstants::ErrorCodes::const_get('DUPLICATED')}") && 17: obj.is_a?(ProjectMedia) && 18: obj.set_fact_check.present? && 19: obj.set_original_claim.present? 20: existing_pm = ProjectMedia.find(JSON.parse(e.message)['data']['id']) (byebug) e #<RuntimeError: No permission to create FeedInvitation> (byebug) JSON.parse(e) *** TypeError Exception: no implicit conversion of RuntimeError into String nil (byebug) JSON.parse(e.message) *** JSON::ParserError Exception: unexpected token at 'No permission to create FeedInvitation' nil

caiosba · 2024-10-30T23:19:28Z

app/graph/mutations/graphql_crud_operations.rb

+      obj.is_a?(ProjectMedia) &&
+      obj.set_fact_check.present? &&
+      obj.set_original_claim.present?
+        existing_pm = ProjectMedia.find(JSON.parse(e.message)['data']['id'])


Related to my other comment: this JSON.parse could be at the top and then the parsed data object could be used to check for the id here and for the error code at the top.

caiosba · 2024-10-30T23:25:35Z

app/models/project_media.rb

+  def self.handle_fact_check_for_existing_claim(existing_pm,new_pm)
+    if existing_pm.fact_check.blank?
+      existing_pm.append_fact_check_from(new_pm)
+    elsif existing_pm.fact_check.present?
+      if existing_pm.fact_check.language != new_pm.set_fact_check['language']
+        new_pm.replace_with_blank_media
+      end
+    end
+  end
+
+  def append_fact_check_from(new_pm)
+    self.set_claim_description = new_pm.set_claim_description
+    self.set_fact_check = new_pm.set_fact_check
+    self.create_claim_description_and_fact_check
+    self
+  end
+
+  def replace_with_blank_media
+    m = Blank.create!
+    self.set_original_claim = nil
+    self.media_id = m.id
+    self.save!
+    self
+  end


Suggestion for this whole block... I think it's more clear to:

Explicitly say, after line 435, that we're returning the existing item: return existing_pm

Explicitly say, after line 438, that we're returning the new item: return new_pm

And then in the methods append_fact_check_from and replace_with_blank_media, you don't need to return self in their last lines (447 and 455).

caiosba reviewed Oct 11, 2024

View reviewed changes

test/controllers/graphql_controller_12_test.rb Show resolved Hide resolved

caiosba reviewed Oct 11, 2024

View reviewed changes

vasconsaurus force-pushed the 5441-create-imported-fact-check-item-exists branch 3 times, most recently from 908c461 to 7dc8545 Compare October 22, 2024 18:36

caiosba requested changes Oct 24, 2024

View reviewed changes

vasconsaurus force-pushed the 5441-create-imported-fact-check-item-exists branch from 7dc8545 to d9dca69 Compare October 28, 2024 18:15

vasconsaurus added 11 commits October 29, 2024 09:31

update how we create project media in sample data

37273b7

so we are able to send an url

add tests

4c43caf

should append FactCheck to ProjectMedia should create a FactCheck with a Blank ProjectMedia

add assetions

be10e6a

first draft: add conditional when we try to create ProjectMedia

6999769

This shouldn't be StandardError, but will keep it for now.

start working on handle_fact_check_for_existing_claim

5454b92

fix test

7828bc8

it fails because of those two assertions, otherwise it passes looks like a syntax error, will look into it Monday

WIP: Create a fact-check, associated with a blank media

a82d016

If the item already has a fact-check in a different language Currently broken

add pt to team languages

4f08234

move setting the claim and fact check

775e54c

tentative unit tests: duplicate original claims

b504883

add unit tests to reflect current behavior

change Sidekiq::Testing.inline! to fake!

63a0725

vasconsaurus added 8 commits October 29, 2024 09:31

get existing project media from the error and send it

17117fc

WIP: change instance to class method

7e36c7e

for this to work I had to change create_claim_description_and_fact_check to be public

move begin/rescue and update our second case

d4f6e67

try to make it easier to read

49cbdeb

change StandardError to RuntimeError and check error message

6dd9043

fix tests

8c3f69b

remove whitespace

9a38f1b

undo changes to sample data

8b364f0

The changes I added to sample_data had come side effects to other tests. So I'm rolling that back, and updated the two graphql tests to not need those changes anymore.

vasconsaurus force-pushed the 5441-create-imported-fact-check-item-exists branch from 9771c6d to 8b364f0 Compare October 29, 2024 13:36

Update how we check the error message

9944a3f

The error can be slightly different, parsing the way we were before doesn't work for every error. This caused test test/controllers/graphql_controller_12_test.rb:69, GraphqlController12Test#test_should_not_create_feed_invitation, to break.

vasconsaurus requested a review from caiosba October 29, 2024 18:15

caiosba requested changes Oct 29, 2024

View reviewed changes

vasconsaurus added 3 commits October 30, 2024 08:13

change the name according to reviewer's suggestion

2884671

Since this method takes an action, I suggest to name it with the verb name in imperative form

check the lapis code error instead of text message

cb97946

(1) If I18n.locale is not English, then this error message will be localized, so won't match this condition (for example, for Portuguese it will be "Este item já existe"). (2) The copy for the error message may change in the future.

change hardcoded 9 to LapisConstants::ErrorCodes::const_get('DUPLICAT…

523a147

…ED')

vasconsaurus changed the title ~~[WIP] 5441 – Change how we create imported fact-checks when an item already exists~~ 5441 – Change how we create imported fact-checks when an item already exists Oct 30, 2024

vasconsaurus marked this pull request as ready for review October 30, 2024 18:29

vasconsaurus requested review from melsawy, DGaffney and jayjay-w as code owners October 30, 2024 18:29

vasconsaurus requested a review from caiosba October 30, 2024 18:29

caiosba approved these changes Oct 30, 2024

View reviewed changes

reviewer suggestion: make what we are returning explicit

61ee8f3

vasconsaurus merged commit 06b32dc into develop Oct 31, 2024
11 checks passed

vasconsaurus deleted the 5441-create-imported-fact-check-item-exists branch October 31, 2024 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

5441 – Change how we create imported fact-checks when an item already exists #2079

5441 – Change how we create imported fact-checks when an item already exists #2079

vasconsaurus commented Oct 11, 2024 •

edited

Loading

vasconsaurus commented Oct 11, 2024

caiosba Oct 11, 2024

caiosba commented Oct 11, 2024

vasconsaurus commented Oct 16, 2024

caiosba commented Oct 16, 2024 •

edited

Loading

jayjay-w commented Oct 17, 2024

vasconsaurus commented Oct 17, 2024

vasconsaurus commented Oct 18, 2024

caiosba commented Oct 18, 2024

vasconsaurus commented Oct 22, 2024

caiosba commented Oct 22, 2024 •

edited

Loading

caiosba Oct 24, 2024

caiosba Oct 24, 2024

caiosba Oct 24, 2024

caiosba Oct 24, 2024

caiosba Oct 24, 2024

vasconsaurus commented Oct 25, 2024

caiosba commented Oct 25, 2024

vasconsaurus commented Oct 29, 2024

caiosba left a comment

caiosba Oct 29, 2024

vasconsaurus Oct 30, 2024

caiosba Oct 30, 2024

caiosba Oct 29, 2024

caiosba Oct 29, 2024

vasconsaurus commented Oct 30, 2024

caiosba left a comment

caiosba Oct 30, 2024

vasconsaurus Oct 31, 2024

caiosba Oct 30, 2024

caiosba Oct 30, 2024

	def is_unique
	obj_name = 'media'
	obj = ProjectMedia.where(team_id: self.team_id, media_id: self.media_id).last
	unless obj.nil?
	error = {
	message: I18n.t("#{obj_name}_exists", team_id: obj.team_id, id: obj.id),
	code: LapisConstants::ErrorCodes::const_get('DUPLICATED'),
	data: {
	team_id: obj.team_id,
	type: obj_name,
	id: obj.id,
	url: obj.full_url
	}
	}
	raise error.to_json
	end
	end

5441 – Change how we create imported fact-checks when an item already exists #2079

5441 – Change how we create imported fact-checks when an item already exists #2079

Conversation

vasconsaurus commented Oct 11, 2024 • edited Loading

Description

Wanted Behavior

Context

The issue with this

A related issue

How has this been tested?

Things to pay attention to during code review

Checklist

vasconsaurus commented Oct 11, 2024

Choose a reason for hiding this comment

caiosba commented Oct 11, 2024

vasconsaurus commented Oct 16, 2024

caiosba commented Oct 16, 2024 • edited Loading

jayjay-w commented Oct 17, 2024

vasconsaurus commented Oct 17, 2024

vasconsaurus commented Oct 18, 2024

caiosba commented Oct 18, 2024

vasconsaurus commented Oct 22, 2024

caiosba commented Oct 22, 2024 • edited Loading

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

vasconsaurus commented Oct 25, 2024

caiosba commented Oct 25, 2024

vasconsaurus commented Oct 29, 2024

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

vasconsaurus commented Oct 30, 2024

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

vasconsaurus commented Oct 11, 2024 •

edited

Loading

caiosba commented Oct 16, 2024 •

edited

Loading

caiosba commented Oct 22, 2024 •

edited

Loading