-
Notifications
You must be signed in to change notification settings - Fork 7
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
Update Feast model to draw data from new card meta #1612
Conversation
c8a78f6
to
484381b
Compare
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.
Looks good!
Enabling some unused warnings from the scala compiler shows a number of unused imports and parameters – it’d be worth us cleaning those up in a subsequent PR.
@@ -29,7 +30,20 @@ class FeastPublicationTarget(snsClient: AmazonSNS, config: ApplicationConfigurat | |||
bio = metadata.flatMap(_.bio), | |||
backgroundHex = metadata.flatMap(_.theme.map(_.palette.backgroundHex)), | |||
foregroundHex = metadata.flatMap(_.theme.map(_.palette.foregroundHex))) | |||
case _:EditionsFeastCollection => FeastCollection(byline=None, darkPalette=None, image=None, body=None, title="", lightPalette=None, recipes=List.empty) | |||
case EditionsFeastCollection(id, addedOn, metadata) => |
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.
The id
and addedOn
pattern variables here are unused (confirmed by adding -Wunused:patvars
to scalacOptions), as is the addedOn
variable on line 35. Just wanted to confirm that’s intended! (I’d be tempted to prefix their names with an underscore to indicate they’re deliberately not used.)
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.
Intentional as there's no place in the downstream Feast model for these values, AFAIK (ids are for the Fronts relational model, and Feast doesn't care when these items were initially curated), so I've cleaned up as you've suggested in ca5758a.
I should note I haven’t tested this on CODE yet, and from the bullet point in the PR description it sounds like you haven’t either. We should do that before merge: I’ll try to get to it tomorrow. |
Ok, deployed to CODE, created an issue, published it, saw no errors in the logs. All good! One behaviour surprised me: I can drag recipes into a collection from the clipboard, but not from the search result pane on the left. I assume that’s already known about and we have a trello card to fix it, but I wanted to mention it just in case. |
Apologies, should have updated the PR description – this was tested in CODE, and thank you for testing, too! Spot on re: compiler warnings. It looks like you've raised a draft PR for that: thank you! Happy to take that forward, or would you like to continue? > One behaviour surprised me: I can drag recipes into a collection from the clipboard, but not from the search result pane on the left. I assume that’s already known about and we have a trello card to fix it, but I wanted to mention it just in case. This is a bug, thank you for spotting! This PR has a clear backend focus, so we'll card and fix in another PR. |
Seen on PROD (merged by @jonathonherbert 6 minutes and 23 seconds ago) Please check your changes! |
No strong feelings! If you want to, go for it – I’ll get to it when I have some time. |
What's changed?
Updates our Feast model to draw data from the new card meta introduced in #1589, #1607, and #1609.
How to test
recipes.code.dev-guardianapis.com/feast-northern-hemisphere/all-recipes/2024-08-01/curation.json
For example, for the input:
We get the output:
Expand to see JSON
Checklist
General
Client