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

Allow creating archived bookmark through REST API #268

Merged
merged 5 commits into from
May 26, 2022

Conversation

kencx
Copy link
Contributor

@kencx kencx commented May 24, 2022

This PR implements a POST method for the /api/bookmarks/archived endpoint that creates an archived bookmark directly.

It would be helpful for #243.

bookmarks/api/routes.py Outdated Show resolved Hide resolved
bookmarks/api/routes.py Outdated Show resolved Hide resolved
bookmarks/tests/test_bookmarks_api.py Outdated Show resolved Hide resolved
bookmarks/tests/test_bookmarks_api.py Outdated Show resolved Hide resolved
bookmarks/api/routes.py Outdated Show resolved Hide resolved
@kencx
Copy link
Contributor Author

kencx commented May 25, 2022

I added the optional is_archived flag to the existing POST endpoint and also ensured that the test verifies all fields.
I will also update the docs after the code has been reviewed, to prevent unnecessary changes.

One problem I foresee is that POST-ing an existing normal bookmark with the is_archived: True flag will archive it, but the reverse is not true, ie. an existing archived bookmark will not be unarchived with the is_archived: False flag.

Of course, there is the archive and unarchive endpoint for this but I feel it makes sense to standardize this behaviour. If you feel the same, I can add this.

bookmarks/api/serializers.py Outdated Show resolved Hide resolved
bookmarks/tests/test_bookmarks_api.py Show resolved Hide resolved
@sissbruecker
Copy link
Owner

sissbruecker commented May 26, 2022

One problem I foresee is that POST-ing an existing normal bookmark with the is_archived: True flag will archive it, but the reverse is not true, ie. an existing archived bookmark will not be unarchived with the is_archived: False flag.

With the current changes, the is_archived flag will not be considered at all on updates, since it is not used in the BookmarkSerializer.update method. Overwriting a bookmark's is_archived flag in there could lead to unarchiving an archived bookmark if the is_archived flag is not specified in the payload, which doesn't feel right. I think it's OK to exclude updates from this PR and rely on having the archive / unarchive endpoints for now.

@kencx
Copy link
Contributor Author

kencx commented May 26, 2022

All proposed changes made and I have updated the docs too.

@sissbruecker
Copy link
Owner

Looks good, updated the API docs a bit

@sissbruecker sissbruecker merged commit 792a19d into sissbruecker:master May 26, 2022
@sissbruecker sissbruecker changed the title Add POST archived API endpoint Allow creating archived bookmark through REST API May 26, 2022
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