Skip to content
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

Rename the ContentProvider model to ContentSource #4346

Closed
krysal opened this issue May 16, 2024 · 1 comment · Fixed by #4402
Closed

Rename the ContentProvider model to ContentSource #4346

krysal opened this issue May 16, 2024 · 1 comment · Fixed by #4402
Assignees
Labels
💻 aspect: code Concerns the software code in the repository 🛠 goal: fix Bug fix 🟧 priority: high Stalls work on the project or its dependents 🧱 stack: api Related to the Django API

Comments

@krysal
Copy link
Member

krysal commented May 16, 2024

Problem

In #4238, a confusing situation was raised when we found that the items of the ContentProvider model actually correspond to sources, not providers. We want to correct this naming so the usage of this model is clear and safe.

Description

Here I copy @obulat's comment explaning the situation:

Originally, we only had source. Then, the provider field was added to denote the provider of the metadata about the image 1. The PR that added the provider field, didn't touch the ContentProvider model, and this model wasn't updated later.

ContentProvider currently only handles sources, not providers. You can check this if you go to https://openverse.org/sources - you'll see NASA in the table. This uses the stats endpoint that queries sources, not providers.

All this is to say that I think we can proceed with this PR, and exactly because we rely on ContentProvider to filter sources. I'll review the code again to check that this is true.

...

Could we open a new issue (maybe even a Project proposal request?) to

By the way, some of the issues mention that a source can appear in several providers. I think we have a project for implementing this, this would reduce the number of duplicates we have, but we would need to find a way to handle a work that has several providers.

Footnotes

  1. https://github.com/cc-archive/cccatalog-api/issues/531

@krysal krysal added 🟨 priority: medium Not blocking but should be addressed soon 🛠 goal: fix Bug fix 💻 aspect: code Concerns the software code in the repository 🧱 stack: api Related to the Django API labels May 16, 2024
@sarayourfriend
Copy link
Contributor

We need to rename the serializer, fix the documentation, and update variable names as well. The model name is not the only place that has this issue. I've discussed that in this comment: #4238 (comment)

I'd also recommend we only change the API-level code, rather than going through the hassle of updating the table name at this time. Doing just the model and code level changes makes this a much easier and less tedious change to implement than if we also included changing the table name.

@krysal krysal added 🟧 priority: high Stalls work on the project or its dependents and removed 🟨 priority: medium Not blocking but should be addressed soon labels May 21, 2024
@obulat obulat self-assigned this May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository 🛠 goal: fix Bug fix 🟧 priority: high Stalls work on the project or its dependents 🧱 stack: api Related to the Django API
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants