Skip to content
This repository has been archived by the owner on Aug 4, 2023. It is now read-only.

Modify audio columns #130

Merged
merged 8 commits into from
Jul 31, 2021
Merged

Modify audio columns #130

merged 8 commits into from
Jul 31, 2021

Conversation

krysal
Copy link
Member

@krysal krysal commented Jul 20, 2021

In line with WordPress/openverse-api#141, this PR:

  • Add watermarked column to the schema of the audio table in SQL files
  • Rename the genre column to genres and changes its type to a Postgres array field
  • Rename the alt_audio_files column to alt_files
  • Keep the old standardized_popularity column for image and audio tables.

@krysal krysal requested a review from a team as a code owner July 20, 2021 04:56
@krysal krysal requested review from zackkrida and dhruvkb July 20, 2021 04:56
Copy link
Member

@dhruvkb dhruvkb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

view_count is also a non-nullable field in media. The sample data for images (in openverse-api) contains a view_count column with all values set to zero, can this be reproduced for audio?

@@ -73,7 +73,7 @@
name='category', required=False, size=80, truncate=False,
),
columns.JSONColumn(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a string array column. In PostgreSQL they are represented by varchar(<len>)[] and the data can be stringified as {<item 1>,<item 2>...}.

The current implementation of genres is varchar(80)[].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on this, it's needed to add the new column type.

@krysal krysal force-pushed the modify_audio_columns branch from b0f3d3f to 2bdb6ec Compare July 21, 2021 00:44
@krysal krysal force-pushed the modify_audio_columns branch from 2bdb6ec to 9895bf2 Compare July 21, 2021 03:13
@krysal
Copy link
Member Author

krysal commented Jul 21, 2021

I'm not sure how view_count will fit within the audio table, is not in the schema of the image table, only in the old one and apparently it's not used.

@dhruvkb
Copy link
Member

dhruvkb commented Jul 21, 2021

If it's no longer used, should/can we remove it from the API models and sample data as well?

I think it's best to remove fields that are no longer used, but we should loop in @obulat and @zackkrida for their opinions on this.

@zackkrida
Copy link
Member

I agree that we should remove view_count, it can be safely removed from everywhere it appears.

CleanShot 2021-07-27 at 11 30 08@2x

@krysal krysal force-pushed the modify_audio_columns branch from 27c1ca4 to 182e159 Compare July 29, 2021 15:44
@krysal
Copy link
Member Author

krysal commented Jul 29, 2021

This is ready for review, can be tested with an audio API provider script, like Jamendo, and should load the audio table with the columns changed.

For example, the genres column as an array of character varying(80):

Screen Shot 2021-07-29 at 11 12 54 AM

@krysal krysal requested a review from dhruvkb July 29, 2021 16:05
Copy link
Member

@dhruvkb dhruvkb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 (We can remove the view_counts in a separate PR later on)

@krysal krysal merged commit d1dde6f into main Jul 31, 2021
@krysal krysal deleted the modify_audio_columns branch July 31, 2021 02:26
@zackkrida zackkrida added the ✨ goal: improvement Improvement to an existing user-facing feature label Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
✨ goal: improvement Improvement to an existing user-facing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants