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

Fix the bookmark API and add tests for it #1425

Merged
merged 7 commits into from
Feb 7, 2025

Conversation

BentiGorlich
Copy link
Member

  • Add the bookmarks property to entry, entry_comment, post and post_comment responses
  • Fix problem with scopes
  • makeDefault API endpoint now returns the list
  • The response from editing a list now contains the correct values
  • Add tests for bookmarking entry, entry_comment, post and post_comment to the default list and a specific list
  • Add tests for creating, editing and deleting lists as well as making it default

- Add the `bookmarks` property to entry, entry_comment, post and post_comment responses
- Fix problem with scopes
- `makeDefault` API endpoint now returns the list
- The response from editing a list now contains the correct values
- Add tests for bookmarking entry, entry_comment, post and post_comment to the default list and a specific list
- Add tests for creating, editing and deleting lists as well as making it default
@BentiGorlich BentiGorlich added bug Something isn't working backend Backend related issues and pull requests testing Unit/Functional/Integration test issues and pull requests api API related issues and pull requests labels Feb 4, 2025
@BentiGorlich BentiGorlich requested review from melroy89 and jwr1 February 4, 2025 14:18
@BentiGorlich BentiGorlich self-assigned this Feb 4, 2025
@melroy89
Copy link
Member

melroy89 commented Feb 4, 2025

Deployed on kbin.melroy.org atm

Copy link
Member

@jwr1 jwr1 left a comment

Choose a reason for hiding this comment

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

Could it be made so the default when you're signed out is null, to indicate that you couldn't have any bookmarks anyway? But when you're signed in, it keeps the empty array (assuming it's not in a list).

@jwr1
Copy link
Member

jwr1 commented Feb 4, 2025

For some reason, the new bookmarks property is showing up in the API docs for the EntryResponse, but not for posts, entry-comments, or post-comments. When actually testing the API, it does look like the property is there though.

@jwr1
Copy link
Member

jwr1 commented Feb 4, 2025

makeDefault API endpoint now returns the list

Maybe the docs just didn't get updated correctly, because the API docs still says the makeDefault route doesn't return anything.

Copy link
Member

@jwr1 jwr1 left a comment

Choose a reason for hiding this comment

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

Could each of the 4 bookmark adding and removing routes return the new list of bookmarks?

The main issue I see is that a post might have a known list of bookmark lists that it is in, then if the Add Bookmark to Default List route is used, I know the post was added to a Bookmark List, but I don't know which one without either manually checking which one is the default and the guessing that's the one it was added to, or refetching the post's information from the API, just to update its bookmarks property.

@melroy89
Copy link
Member

melroy89 commented Feb 4, 2025

makeDefault API endpoint now returns the list

Maybe the docs just didn't get updated correctly, because the API docs still says the makeDefault route doesn't return anything.

Be sure to check https://kbin.melroy.org/api/docs. Other documentation pages will only be update later if it's merged.

@jwr1
Copy link
Member

jwr1 commented Feb 4, 2025

makeDefault API endpoint now returns the list

Maybe the docs just didn't get updated correctly, because the API docs still says the makeDefault route doesn't return anything.

Be sure to check https://kbin.melroy.org/api/docs. Other documentation pages will only be update later if it's merged.

Yup, that's what I was using.

@melroy89
Copy link
Member

melroy89 commented Feb 4, 2025

makeDefault API endpoint now returns the list

Maybe the docs just didn't get updated correctly, because the API docs still says the makeDefault route doesn't return anything.

Be sure to check https://kbin.melroy.org/api/docs. Other documentation pages will only be update later if it's merged.

Yup, that's what I was using.

OK great. That is good.

This is what it shows:

image

The 200 OK response only says: "Sets the provided list as the default". There is no type definition added, so no response schema. And no example.

So you only see this description text. If that is not sufficient and you want more details. Then this file needs to be extended as well:

Currently the content is set to null...

@melroy89 melroy89 added this to the v1.8.0 milestone Feb 4, 2025
- Adjust tests for retrieving content to assert the correct values for the bookmarks (null if not logged in, empty if logged in)
- fix OpenAPI docs to list the correct responses
- Add response to all bookmark routes so the new list of bookmarks the subject belongs to is returned
- adjust the tag of the bookmark and bookmark-list routes
@BentiGorlich BentiGorlich requested a review from jwr1 February 5, 2025 09:33
@melroy89
Copy link
Member

melroy89 commented Feb 5, 2025

kbin.melroy.org is rebased. makeDefault does now have a content and swagger will show:

image

If this is an array instead of a single object? Then the PHP code still needs to be adapted.

@jwr1
Copy link
Member

jwr1 commented Feb 5, 2025

Two more things:

  • The api docs looks mostly correct now, but I did see the EntryResponseDto still has the bookmarks property marked as required, even though it is optional.
  • Whenever I try to use the /api/bookmark-lists route, all I'm getting is a 403 Forbbiden error (and I do have the user oauth scope).

@BentiGorlich
Copy link
Member Author

Did you use the correct scope? Because there are new ones

@jwr1
Copy link
Member

jwr1 commented Feb 5, 2025

Do you mean the user:bookmark_list scope, because I thought the user scope covered anything that was prefixed with user:?

@BentiGorlich
Copy link
Member Author

Yeah, I think it doesn't... Getting to know these scopes and I think it just doesn't belong in the user scope and I think this inheritance is not happening automagically

jwr1
jwr1 previously approved these changes Feb 5, 2025
Copy link
Member

@jwr1 jwr1 left a comment

Choose a reason for hiding this comment

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

Adding the user:bookmark_list scope did fix it.

I'm still testing a few more things, but so far I'd say everything is looking good now.

@melroy89
Copy link
Member

melroy89 commented Feb 5, 2025

BookmarkListDto2 schema on request body is missing on PUT /api/bookmark-lists/{list_name}:

image

@melroy89
Copy link
Member

melroy89 commented Feb 6, 2025

I will move away from this branch, just saying @jwr1 . In order to validate some other PRs.

@BentiGorlich
Copy link
Member Author

I will fix the scope thing and the missing response type :)

- fix the scopes so the `bookmark` and `bookmark_list` scopes do not belong to the `user` scope anymore
- Make the 2 routes that deletes bookmarks use the `DELETE` request type
- Fix missing payload in the API docs for editing a list
@BentiGorlich BentiGorlich force-pushed the fix/bookmark-api-and-tests branch from aaf1fdc to 9d1cbc6 Compare February 7, 2025 09:31
@BentiGorlich BentiGorlich merged commit 3bf5d1e into main Feb 7, 2025
7 checks passed
@BentiGorlich BentiGorlich deleted the fix/bookmark-api-and-tests branch February 7, 2025 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API related issues and pull requests backend Backend related issues and pull requests bug Something isn't working testing Unit/Functional/Integration test issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants