6064 – Refactor duplicated media creation methods #2223

vasconsaurus · 2025-02-17T21:37:54Z

Description

The Media class and the ProjectMediaCreators concern, in Check API, have some duplicated logic around creating different types of media. The main differences were:

the ones in the Media class handle original_claim
the ones in ProjectMediaCreators expected a ProjectMedia object,
and the ones in the Media class expected the claim/url/file

References: CV2-6064

How to test?

Because Media is such an important class, we should run the whole suite. But while working on this I mainly ran:

# media tests
rails test test/models/media_test.rb

# has tests that call set_media, which call the code we are changing
rails test test/models/bot/smooch_3_test.rb

# have tests related to original claim
rails test test/models/project_media_7_test.rb
rails test test/controllers/graphql_controller_11_test.rb
rails test test/controllers/graphql_controller_12_test.rb

Checklist

I have performed a self-review of my code and ensured that it is safe and runnable, that code coverage has not decreased, and that there are no new Code Climate issues. I have also followed Meedan's internal coding guidelines.

vasconsaurus · 2025-02-19T17:23:27Z

Some notes on the current state of things

I tried to merge the logic but there are slight differences, that I'm not too sure on what the best way to handle is,
- so there is still some pretty obvious duplication,
- which I thought was best to leave for now: I think it's easier to see the differences/similarities, so it should be easier for us to discuss possible solutions?
the only tests currenlty failing are the two test in rails test test/models/concerns/project_media_creators_test.rb. As they should be, since we no longer have the create_link method inside project_media_creators. Not sure what the best course of action regarding this one is.
I don't like how I'm sending the data from ProjectMedia to Media?
- We need the claim/link/file, for different types of Media we might need different data from ProjectMedia, we need to keep track if it is an original_claim, currently we are not setting the user.. I think there should be a better way to access this?
I kept only create_media and removed create_original_claim from ProjectMediaCreators which seems to make more conceptual sense to me, since in the end it is just creating a media.

I think this is the bulk of it, though for sure I'm forgetting something.

caiosba · 2025-02-20T12:51:24Z

app/models/media.rb

+    if original_claim
+      case media_type
+      when 'UploadedImage', 'UploadedVideo', 'UploadedAudio'
+        m = find_or_create_uploaded_file_media(original_claim, media_type, true)
+      when 'Claim'
+        m = find_or_create_claim_media(original_claim, nil, true)
+      when 'Link'
+        m = find_or_create_link_media(original_claim, team, true)
+      when 'Blank'
+        m = Blank.create!
      end
    else
-      find_or_create_claim_media_from_original_claim(claim)
+      case media_type
+      when 'UploadedImage', 'UploadedVideo', 'UploadedAudio'
+        m = find_or_create_uploaded_file_media(project_media.file, media_type)
+      when 'Claim'
+        m = find_or_create_claim_media(project_media.quote, project_media.quote_attributions)
+      when 'Link'
+        m = find_or_create_link_media(project_media.url, team)
+      when 'Blank'
+        m = Blank.create!
+      end


Let's try to resolve this duplication by trying to send the media information as an argument. Suggested signature:

find_or_create_media_from_content(type, content, additional_args = [])

I moved the methods to Media so it's easier to compare them side-by-side, and to make sure this works from Media. I also made a small change to the create_media method in ProjectAssociation, so its behavior is closer to create_original_claim. Since those two have differences, but are both basically creating a Media associated to the ProjectMedia. Hopefully those two will be turned into one method.

failing tests: rails test test/models/project_group_test.rb:39 rails test test/models/project_media_5_test.rb:799 rails test test/controllers/graphql_controller_7_test.rb:370 rails test test/controllers/graphql_controller_5_test.rb:200 - we make sure original_claim is not nil before trying to match - after that was fixed, project_media_5_test.rb:799 was still failing: - we werent passing team - so it failed to get the pender_key, since that expects a team - questions: - why was this working before? we expected team before as well - do we need to have team?

…aim the same

One big difference between how we create Media is in one case we were sending the entire project_media, but when creating from original_claim we weren't. I think since this is Media it makes more sense only to receive what we need, instead of the whole project_media. So this is a step in that direction. There is still a lot of duplication, and I think there should be better ways to access the values we want.

The method signature changed, so I updated the tests. however: should it change? is there a better way to deal with this?

create_media checked if media_type.blank? before running, however, the graphql request might send original_claim and media_type set to Blank. Which means in the case of original_claim present we do want to check and re-set media_type if needed.

The issue here, I think, is create_project_media creates a bunch of stuff by default. So it was creating an extra Link Media, which caused some confusion on when debugging a different issue. So maybe it's a good idea to be more conservative in using sample_data's methods?

They are now both doing the same thing. And I think conceptually they both create media, even if it is a media from an original claim.

To make clear this method mutates the original object.

Depending on the type of Media we try to create, it needs different data from the ProjectMedia object. Still, Media shouldn't need to know too much about ProjectMedia. So, we are deciding on what to send in ProjectMediaCreators, and only sending what is necessary: - I created a method that gets those media_arguments - Moved that and set_media_type under protected - Inside media it only gets that data and forwards it - There were a few changes to method's signatures (so tests were also updated) - Because there are a few differences between arguments needed, we are sending those as an additional_args hash

create_link is no longer in ProjectMediaCreators, only in Media. I think these two tests are already covered in media_test.rb

codeclimate · 2025-02-24T15:10:33Z

Code Climate has analyzed commit 09625ae 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 · 2025-02-26T12:56:26Z

@melsawy @jayjay-w please review this as well.

caiosba

Thanks Manu for addressing the things we discussed before! I left a couple of comments, but they are not blockers.

caiosba · 2025-02-26T15:41:39Z

app/models/media.rb

+  # we set it to UploadedImage by default, should we?
+  # def self.create_with_file(project_media, media_type = 'UploadedImage')


We can remove these comments now, right?

Yes! But should we set media_type = 'UploadedImage' by default?

No, I don't think it makes sense to have any default.

caiosba · 2025-02-26T15:46:39Z

test/controllers/graphql_controller_12_test.rb

@@ -870,7 +870,7 @@ def teardown
    WebMock.stub_request(:get, pender_url).with({ query: { url: url } }).to_return(body: response_body)

    t = create_team
-    pm = create_project_media team: t, set_original_claim: url
+    pm = ProjectMedia.create(team: t, set_original_claim: url)
    cd = create_claim_description project_media: pm


I suggest you replace ProjectMedia.create by ProjectMedia.create! - just to make it more verbose if it fails.

To make it more verbose if it fails.

melsawy · 2025-02-26T18:42:45Z

app/models/concerns/project_association.rb

-        m = self.create_media
-        self.media_id = m.id unless m.nil?
+        self.create_media!
+        self.media_id unless self.media_id.nil?


Q: this line should return self.media_id or assign a value to self.media_id

The create_media method changes the object and assigns the value, if I'm not mistaken. So I think we should be able to just return self.media_id.

vasconsaurus changed the title ~~6064 refactor media creation methods~~ [WIP] 6064 – Refactor duplicated media creation methods Feb 19, 2025

caiosba reviewed Feb 20, 2025

View reviewed changes

vasconsaurus added 21 commits February 24, 2025 09:27

[WIP] make create_media and create_original_claim similar

fa98310

merge methods to create claim into one

ac8664e

extract setting the media_type

574319b

merge methods to create uplodaded media and fix a variable name

9bc9271

merge create_link methods

f194c1f

do some cleanup

371bf03

update methods names

e5f58d6

move set_media_type back and make create_media and create_original_cl…

d914b85

…aim the same

Fix failing tests

fe841ae

The method signature changed, so I updated the tests. however: should it change? is there a better way to deal with this?

update to keep create_media only, instead of create_original_claim

3d2f767

They are now both doing the same thing. And I think conceptually they both create media, even if it is a media from an original claim.

add ! to method name

a2a97db

To make clear this method mutates the original object.

try to deal with codeclimate complains

d4ebac0

fix indentation

7507a0a

try to make this a bit clearer

00dd298

remove deprecated tests

09625ae

create_link is no longer in ProjectMediaCreators, only in Media. I think these two tests are already covered in media_test.rb

vasconsaurus force-pushed the 6064-refactor-media-creation-methods branch from bd631f1 to 09625ae Compare February 24, 2025 13:29

vasconsaurus changed the title ~~[WIP] 6064 – Refactor duplicated media creation methods~~ 6064 – Refactor duplicated media creation methods Feb 25, 2025

vasconsaurus marked this pull request as ready for review February 25, 2025 14:03

vasconsaurus requested review from jayjay-w and melsawy as code owners February 25, 2025 14:03

vasconsaurus requested a review from caiosba February 25, 2025 17:51

caiosba approved these changes Feb 26, 2025

View reviewed changes

vasconsaurus added 2 commits February 26, 2025 14:52

clean up

d47e1ef

replace ProjectMedia.create by ProjectMedia.create!

5b99440

To make it more verbose if it fails.

melsawy reviewed Feb 26, 2025

View reviewed changes

melsawy approved these changes Feb 26, 2025

View reviewed changes

vasconsaurus merged commit 2bf3532 into develop Feb 27, 2025
13 of 14 checks passed

vasconsaurus deleted the 6064-refactor-media-creation-methods branch February 27, 2025 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6064 – Refactor duplicated media creation methods #2223

6064 – Refactor duplicated media creation methods #2223

vasconsaurus commented Feb 17, 2025 •

edited

Loading

vasconsaurus commented Feb 19, 2025 •

edited

Loading

caiosba Feb 20, 2025

codeclimate bot commented Feb 24, 2025

caiosba commented Feb 26, 2025

caiosba left a comment

caiosba Feb 26, 2025

vasconsaurus Feb 26, 2025

caiosba Feb 26, 2025

caiosba Feb 26, 2025

melsawy Feb 26, 2025

vasconsaurus Feb 27, 2025

		# we set it to UploadedImage by default, should we?
		# def self.create_with_file(project_media, media_type = 'UploadedImage')

6064 – Refactor duplicated media creation methods #2223

6064 – Refactor duplicated media creation methods #2223

Conversation

vasconsaurus commented Feb 17, 2025 • edited Loading

Description

How to test?

Checklist

vasconsaurus commented Feb 19, 2025 • edited Loading

caiosba Feb 20, 2025

Choose a reason for hiding this comment

codeclimate bot commented Feb 24, 2025

caiosba commented Feb 26, 2025

caiosba left a comment

Choose a reason for hiding this comment

caiosba Feb 26, 2025

Choose a reason for hiding this comment

vasconsaurus Feb 26, 2025

Choose a reason for hiding this comment

caiosba Feb 26, 2025

Choose a reason for hiding this comment

caiosba Feb 26, 2025

Choose a reason for hiding this comment

melsawy Feb 26, 2025

Choose a reason for hiding this comment

vasconsaurus Feb 27, 2025

Choose a reason for hiding this comment

vasconsaurus commented Feb 17, 2025 •

edited

Loading

vasconsaurus commented Feb 19, 2025 •

edited

Loading