Skip to content

Update our minimal data logic to be a fallback logic #590

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Update our minimal data logic to be a fallback logic #590

wants to merge 13 commits into from

Conversation

vasconsaurus
Copy link
Contributor

@vasconsaurus vasconsaurus commented Sep 3, 2025 •
edited
Loading

Description

We realized we were setting default/fallback values in a way that was a bit scattered around. So we want to centralize this. How I approached it:

At first, we thought about setting defaults, but I decided to treat this as a fallback. The parsers don’t have to worry about checking anything, they can just send back whatever they want. And in the end, we check the response, and add fallbacks where needed.

References: CV2-6500, CV2-6470

How has this been tested?

By running the test suite over and over.

Things to pay attention to during code review

This touches a lot of Media logic, I tried to get it to work, and then tried to clean it up. I think this one requires a lot of attention in general, but here are the main notes:

  • I created a MediaData concern. It has a few methods, and a DATA_STRUCTURE constant that defines the expected structure of the data hash.
    • This helps as documentation, and also ensure all keys are present, because some methods rely on the presence of certain keys.
    • It also enabled me to just pass the URL when building a minimal data object, instead of having to first make a Media object or simulate one (when we call get_error_data)
  • Before, because we cleaned the data if writing to cache, we had to clean it if there was no cache when returning the data at the end. Now we just clean it up front, and write or return the same clean data.
  • I updated the parse method:
    • I'm matching and getting the new Parser instance, instead of looping. I think this makes it easier to read.
    • We were setting the provider, and type as fallbacks in the beginning, and then resetting them to the correct ones inside get_oembed_data. Since provider and type are parser related logic, I moved that inside Parser.
    • I also moved setting the URL inside Parser, which I don't think is ideal. I thought about setting it in Media, but there were issues with that. I think this one will be easier to deal with when we work on separating Media and Parse behavior more. For now, I'm setting it when setting provider and type.
    • Had to make a minor tweak to page parser, because that is the only one that sets a URL value, if the redirected URL requires cookies.
    • This also meant updating get_oembed_data to deal with nil/empty values, because it is called a bunch of times, and we are not setting things up front anymore. By the last time it is called, we have all values present.

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 vasconsaurus force-pushed the 6470-6500-centralize-minimal-data-fallbacks-2 branch from 04d5876 to a3c474b Compare September 5, 2025 18:27
@vasconsaurus
Make sure methods get expected values and fix tests
57765f9 
This is still kind of messy, I just wanted to make sure methods get the
expected values for now. But I should clean up the fallback method,
and the parse method.
@vasconsaurus
Clean up fallback logic
1250226 
I added a DATA_STRUCTURE constant to define the expected structure of
the data hash. This helps as documentation, but we can also initialize
with a deep copy of this structure to ensure all keys are present, because
some methods rely on the presence of certain keys.

I also think fallback and required_fields should be instance methods,
and we shouldn't need a separate minimal_data method anymore.
But we do need it for get_error_data, since I haven't found a better
solution yet, I kept this as a middle ground.
@vasconsaurus
Extract minimal data logic to MediaData class
9a4b443
@vasconsaurus
Update id to be called key
5ea788e 
Since this refers to cache, and cache_key, key is clearer.
@vasconsaurus
Updates medias_helper, media and medias_controller_test
1160b02 
medias_helper: makes sure we merge error_data
media: we only need to return the data we already have, we shouldn't
need to set_fallbacks again
medias_controller_test: use MediaData.required_fields
@vasconsaurus
Pass MediaData.minimal_data a URL instead of the whole media
0a7d86a 
Before, when minimal_data was still a method inside Media, it expetected
the whole media object. After begin refactored as a method inside MediaData,
we can just pass the URL.

Note:
- test "should return data with error message if can't instantiate" is flaky.
And I'm not sure about what it is trying to do. I left it as is for now.
@vasconsaurus
Fix bug related to options.delete(:force)
4cfc286 
Since we deleted it before we parse, when it got to
the archive_if_conditions_are_met method, the options
hash never had the :force key.

This also reads better.
@vasconsaurus
Update parse method
4a62bd0 
- A suggestion: I'm matching and getting the new Parser instance,
instead of looping. I think this makes it easier to read.
- I moved the hashes we create so it is easier to read, but I still don't
like it, it doesn't really solve the issues:
   - why do we have to set the url to parser.url here?
   - I think the parser should set thr provider and type.
@vasconsaurus
Move the setting of provider and type out of oembed
91c8c64 
I'm still not a fan of setting provider, type and url inside the parse
method separately. But, turns out, we were doing this inside oembed,
which I think is worse.

So I'm at least moving that out, and we can rethink it.
@vasconsaurus
Rename clean_json to clean_data
7b0aada 
Not a big fan of the name, but Caio was right all along, and this is not yet a json.
So the name is misleading.
@vasconsaurus
Check for presence before setting the data[key]
29db5d0 
For page_item we check if cookies are present, and set original_url
as the url if they are. So, we make sure we are actually setting the value,
and check for presence before setting it in parse.
@vasconsaurus
Update so that parser returns provider, type and url
d7278b3 
- Also sets a minimal data for parser, as we are doing for media
– We call get_oembed_data at different points, and since we are not setting
everything up front, we now have to deal with items not being present.
@vasconsaurus vasconsaurus force-pushed the 6470-6500-centralize-minimal-data-fallbacks-2 branch from c85d3b1 to d7278b3 Compare September 10, 2025 11:15
@vasconsaurus vasconsaurus marked this pull request as ready for review September 10, 2025 12:25
@vasconsaurus
Copy link
Contributor Author

As of recently, failed code climate upload.

@vasconsaurus
Copy link
Contributor Author

@caiosba

  • When the code is approved, I still need to update the README and diagram.

@caiosba caiosba removed their request for review September 10, 2025 12:32
@caiosba
Copy link
Contributor

caiosba commented Sep 10, 2025

@caiosba

  • When the code is approved, I still need to update the README and diagram.

Sounds good, @vasconsaurus - @melsawy please review this PR.

@melsawy
Copy link

melsawy commented Sep 10, 2025

@melsawy please review this PR.

Sure Caio will do today isA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melsawy melsawy melsawy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vasconsaurus @caiosba @melsawy