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

Add routes for resolving items by refcode #807

Merged
merged 3 commits into from
Jul 31, 2024
Merged

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jul 13, 2024

First steps on #803

This PR adds /items/by-refcode/<refcode> and /items/by-id/<id> endpoints that essentially replace /get_item_data/<item_id>. I'm not entirely happy with the routes yet, and would like to find some examples of how other APIs deal with what are essentially multiple canonical IDs. Having by-id and by-refcode feels natural, but perhaps simply /items/refcode/<refcode> is more extensible and compatible with e.g. JSON:API to other fields in the future.

Went with this in the end: having /items/<refcode> be the canonical URL is appealing, with the "legacy" endpoint being the only way to refer to items by item ID -- this would also allow us to promote refcode at some point to simply be the top-level id field in JSON:API parlance, which has some nice properties.

Copy link

codecov bot commented Jul 13, 2024

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.33%. Comparing base (95d5998) to head (5250762).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #807      +/-   ##
==========================================
+ Coverage   67.26%   67.33%   +0.06%     
==========================================
  Files          62       62              
  Lines        3858     3869      +11     
==========================================
+ Hits         2595     2605      +10     
- Misses       1263     1264       +1     
Files Coverage Δ
pydatalab/pydatalab/permissions.py 84.44% <100.00%> (+0.72%) ⬆️
pydatalab/pydatalab/routes/v0_1/items.py 82.83% <87.50%> (+0.21%) ⬆️

@ml-evs ml-evs marked this pull request as ready for review July 13, 2024 19:44
Copy link

cypress bot commented Jul 13, 2024



Test summary

132 0 0 0


Run details

Project datalab
Status Passed
Commit f1578d5 ℹ️
Started Jul 31, 2024 12:16 PM
Ended Jul 31, 2024 12:20 PM
Duration 04:32 💡
OS Linux Ubuntu -
Browser Multiple

View run in Cypress Cloud ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud

@ml-evs ml-evs force-pushed the ml-evs/refcode-resolver branch 3 times, most recently from f99d6d2 to bb75f2d Compare July 18, 2024 10:49
@ml-evs ml-evs force-pushed the ml-evs/refcode-resolver branch 2 times, most recently from 154c77b to 847a20e Compare July 30, 2024 14:55
Copy link
Contributor

@BenjaminCharmes BenjaminCharmes left a comment

Choose a reason for hiding this comment

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

Looks great !

@ml-evs ml-evs merged commit 51f0486 into main Jul 31, 2024
14 checks passed
@ml-evs ml-evs deleted the ml-evs/refcode-resolver branch July 31, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants