-
Notifications
You must be signed in to change notification settings - Fork 50
Add duration and category to ES mappings #291
Conversation
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, though I don't have any advice on the missing duration piece 😅
The addition of
This is the code that transforms a DB cursor row dict into an |
fcfc43f
to
9ece627
Compare
@dhruvkb Thank you so much! That worked perfectly! I've update the PR with that change and the testing instructions pass now. |
It looks like to make the audio tests pass, the sample responses in the tests need to be updated with the new fields. Other than that this already looks great 👍 |
@@ -119,6 +120,10 @@ class AudioSerializer(MediaSerializer): | |||
required=False, help_text="JSON describing alternative files for this audio." | |||
) | |||
|
|||
category = serializers.CharField( |
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.
I think this needs to be moved to the MediaSerializer now: in the Catalog, we have changed the Category for Audio to match Image Category, i.e. now it's an Array field that can contain several categories.
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.
I'm skeptical about having categories being an array. We already have genres
for many-to-many classification and tags
for attributes. Furthermore, if we look at the example categories (namely music, sound_effect, podcast, news & audiobook), it's hard to see any overlap between them. Even in our sample data, none of the files belong to more than one category.
Anyway if we do decide to change it, here's the Django model for reference. It'll need to be updated.
category = models.CharField( |
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.
As discussed this will stay a string and can be move to the MediaSerializer later.
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.
Should this stay for the time being until the images are converted to a single category? Or is it possible to update it now already?
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.
Maybe @dhruvkb can confirm, I think it'd be better for this to stay where it is for now and for us to update it later but not sure.
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 category can be separate for audio and images for the time being. We can merge the fields later when we clean up the existing image catalog.
Fixes
Fixes #212 by @krysal
Description
Adds the
duration
andcategory
fields to the ES mappings. Also adds thecategory
to the AudioSerializer. Duration was already present there (though doesn't appear to be working in my tests of the audio results endpoint).Testing Instructions
Run
just down && just init
then inspect the response of thehttp://localhost:8000/v1/audio?format=json
endpoint and verify that the expected fields are all present.As I said above, the duration is still missing but I'm unsure why.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin