openzim / cms

ZIM file Publishing Platform
https://cms.openzim.org
GNU General Public License v3.0
4 stars 0 forks source link

Add endpoint books-add #36

Closed anshulxyz closed 2 years ago

anshulxyz commented 2 years ago

Closes #24

codecov[bot] commented 2 years ago

Codecov Report

Merging #36 (24c3573) into main (82a6730) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 24c3573 differs from pull request most recent head 0998188. Consider uploading reports for the commit 0998188 to get more accurate results Impacted file tree graph

@@            Coverage Diff            @@
##              main       #36   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         5    +1     
  Lines          130       166   +36     
=========================================
+ Hits           130       166   +36     
Impacted Files Coverage Δ
backend/src/backend/main.py 100.00% <100.00%> (ø)
backend/src/backend/models.py 100.00% <100.00%> (ø)
backend/src/backend/schemas.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 82a6730...0998188. Read the comment docs.

rgaudin commented 2 years ago

There is some mismatch between the schema mentioned here https://github.com/openzim/cms/issues/24#issuecomment-962984835 vs the Book and BookMetadata model. There are other fields in the metadata example, that are not present in the model Should I add the missing fields to the BookMetadata? such as Publisher, Scraper etc.

While we are designing the API right now, and can bend it to our needs, we are designing it with a clear objective: allowing API users to submit information to our platform and retrieve data from it, in a format that is coherent and makes sense.

It might be tempting to maximize use of the framework's magic and make the API look like our internal data models as to reduce glue code in the CMS but our focus should actually be the other way around: design a clean, coherent API for users and deal with what it means internally. When this and the auto-magic features collide, it's bonus and we should benefit from it, but API design has priority.

What it means here is that, indeed:

Eventually, we want to link to the matching Title (see #25) or create the linked Title, copying the Metadata and Tags (only public ones) to the Title.

anshulxyz commented 2 years ago

hi @rgaudin I have fixed the following (I believe), thoughts?:

we want to create BookMetadata for each entry in the input metadata and link them to the created Book.

Working on the Language and Tags now.

anshulxyz commented 2 years ago

I suppose we can merge this without and you'll add that part to this endpoint when doing #25. Is that OK ?

Yes, it's okay with me. I'll finish the #25 now ASAP

anshulxyz commented 2 years ago

I'd also like the commits to be squashed together into one for the merge, if the policy allows and you see it fit.

rgaudin commented 2 years ago

I'd also like the commits to be squashed together into one for the merge, if the policy allows and you see it fit.

Yes, it's a good fit for this particular case as the changes surface is small and makes sense as a whole.

Now that it's accepted, the process would be for you to rebase and squash into the appropriate number of commits (1 here).

You can do that and force push or I'll squash tomorrow morning.

anshulxyz commented 2 years ago

... squash into the appropriate number of commits ... You can do that and force push

Done