go-shiori / shiori

Simple bookmark manager built with Go
MIT License
9.18k stars 549 forks source link

Add GET endpoint to add bookmarks to use with bookmarklet and other methods #269

Open deanishe opened 4 years ago

deanishe commented 4 years ago

Add GET endpoint for submitting bookmarks via bookmarklet, Web Share API, other integrations.

Acceptance criteria

akavel commented 3 months ago

Hi! I would be interested to try if I could implement this; would you have any pointers for how to approach that for a person not familiar with the codebase? also, how the authentication should work in this case? also, any specifics on how to & how not to implement this? TIA! @fmartingr ?

I would like to try do it in coordination with the amazing maintainers of this project, the way you'd like it to be done, so that you hopefully get more return from this than trouble 😂 I am happy for discussions and back and forth on the PR once it is ready. I'm a maintainer myself, so I know how the things look from the other side. I'd just be grateful for some guidelines for start - for one thing, to begin in roughly good for you direction instead of wasting time on random ideas that would show up be completely against your vision; for the second thing, you know the codebase way better than me so you can easily point to me some initial appropriate places; for the third thing, it could give us a tiny bit better chance at having this maybe actually merged in the future 😂 Anyway, if that's too much effort for you at this moment, I'm totally fine with that too - just please let me know so I can try to dive head-first and just discuss later on a PR (if I even get there). However little or much you can tell me, I'll be grateful - just I'd love rather faster than later if possible 😅❤️ but still ok if not, I will understand. Cheers!

fmartingr commented 3 months ago

Hey @akavel, thank you for volunteering! Some details as how I saw this happening:

This is briefly what I've been thinking about this. Let me know if you have further questions or if you think something would be done in a different way things are obviously up for discussion.

Thanks again!

akavel commented 3 months ago

Thanks @fmartingr for a quick reply! ❤️❤️

One thing that is not super clear to me, both from implementation, but also esp. usage and maybe testing perspective, is how auth should work here? In particular:

fmartingr commented 3 months ago
  • is there some existing mechanism that will automagically work for a new endpoint, or maybe it requires opt-in? this I presume might become obvious in codebase, so here asking just in case; but still:

You only need to add the handler in the bookmark.go file.

  • I saw some mention of jwt somewhere, is that used for auth? is it via cookies? esp. from usage/user-story perspective, for a bookmarklet user, I wonder would they have to re-login very often? or maybe even store user+pwd locally and just send everytime to pre-login and get jwt token every time? I'm actually interested in trying to build a bookmarklet for myself afterwards 😂 so I want to know how I can use it for that, i.e. it's core purpose 😄

Since the main purpose of this endpoint would be using it from a bookmarklet I'd say cookie auth is enough. Authentication is available automatically in all routes via middlewares for both JWT and Cookies.

  • you mention tests with some concern, so I feel a need to ask now, is there some existing testing infra I can plug into and see for inspiration? esp. related to auth? or maybe I should build some part of the infra? (should be ok for me to do anyway, just want to clarify - I presume might also be clear in the code, but asking primarily because of the perceived concern in your tone 😅)

My concern is mostly to try and test all new additions to the codebase because most of the code is currently not covered and since we are undergoing some refactors I expect basic test of the new features/refactors being worked on. Regarding how to test, you can test the handler directly, keep an eye on other tests to check how we did it. We have a bunch of helpers under testutil to ease out asserts done to HTTP handlers, I believe is mainly focused for the API but it can be used/adapted for other handlers.