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 Reading Groups API + Demo view #818

Merged
merged 8 commits into from
Jul 25, 2023
Merged

Conversation

EfronC
Copy link
Contributor

@EfronC EfronC commented Jun 21, 2023

Reading Groups API

Add a feature for Reading Groups to LRR. This is a feature to group archives into an ordered list, so it would be easier to read related works, like separate works from an Story arc.

The code is mainly the API backend code, the frontend files provided is just a simple demo to show an idea for how I made it to work, still I think there might be more efficient designs for the view. I tried to separate all these elements from the base code so that way would be easier to remove them if necessary. The Demo can be checked at: http://localhost:3000/rgroups .

The demo doesn't include a way to add files to the Reading Groups, since the intended way for it to work was to add a button in the context menu of files in the search view, so you can add them directly from there. Said way was not implemented so I didn't have to touch the main code. For testing purposes I made a direct request to the API to add those files manually.

The Edit button in the menu of the Carousel opens a draggable list to reorder the elements, I implemented a simple one from a quick Google search, but I'm pretty sure there are more beautiful ones out there that probably would require new libraries.

PS: Just an aditional comment, The code is not "Complete", I left all endpoints public and didn't set pagination in the lists, for the demo to work.

Copy link
Owner

@Difegue Difegue left a comment

Choose a reason for hiding this comment

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

Thanks a lot for submitting this, and sorry for taking so long to get to reviewing it!

This feature is essentially what I'd drafted in #519, but I never wrote any code for it as it required the now-completed Search revamp and is a pretty heavy workload... I'd like to take this PR as an opportunity to actually start working towards it.

I agree that UI for this is pretty tough to get right, but having an Edit page to create/delete Groups and reorder stuff in them feels like the bare minimum, so you were right to make that one imo!
Playing around with your demo got me thinking about some UI concepts to add archives to groups/categories from the main index, I'll try to draft a spec for that soonish. 👍

I think what we could do here is try focusing towards getting the API finished up + the Edit page if possible? I've added a few comments to try and get us started. 🏃
A fair bit of those are open questions since I'm not 100% on the design, so opinions welcome.

After that, what's left is search and reader integration:

  • hide IDs that are in a group and show the group in the index instead
  • have the group come up when searching for tags that are in any of the included IDs
  • having the reader be able to handle groups by opening the first ID, then the subsequent ones when done reading
  • handling reading progress in a group
  • being able to add groups to categories (not necessary, that'll probably come later imo)

I'll take over afterwards to write those, but if you want to keep helping I wouldn't refuse either 😅

lib/LANraragi/Controller/Api/Readinggroup.pm Outdated Show resolved Hide resolved
lib/LANraragi/Utils/Routing.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/ReadingGroup.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/ReadingGroup.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/ReadingGroup.pm Outdated Show resolved Hide resolved
public/js/rgroups.js Outdated Show resolved Hide resolved
public/js/server.js Show resolved Hide resolved
tools/build/docker/Makefile Outdated Show resolved Hide resolved
templates/rgroups.html.tt2 Outdated Show resolved Hide resolved
lib/LANraragi/Utils/Routing.pm Outdated Show resolved Hide resolved
…odify parameters to signatures - Success Message updated - Add Pagination to GETs
@EfronC
Copy link
Contributor Author

EfronC commented Jul 7, 2023

Some changes applied in most recent commit:

  • Name of the module and all files was changed to Tankoubon.
  • Tanks are now handled with Sorted Sets type in Redis, all modules in the model were modified to work with this. Body of the requests(From both ways) was mostly left intact.
  • Model subs now use signatures instead of '@_'.
  • Success Message for Reorder was modified to match the one in categories.
  • Get Tankoubon list, and Get Tankoubon Elements are now both paginated(Just add a page= at the URI).

There might be some problems that I didn't catch since I just did simple tests with few files, so when the module is considered ready, will be recommendable to test with Tanks with a lot of elements.

PS: Apparently I made a mistake while adding the files in the previous commit, so only one of the changes was inside it, the new commit ↓ is the one containing the changes.

@EfronC
Copy link
Contributor Author

EfronC commented Jul 7, 2023

In this last commit, I add a simple method to search between all the Tanks, and return those which contains the requested file. I added the method in Database.pm, so that way can be used in any place that requires this search. I also added a public Endpoint to use it, so that way the clients can do this themselves for extra features.

I think with this I've already tackled all API related requested changes, so I'll keep in touch for any aditional commentary.

@EfronC
Copy link
Contributor Author

EfronC commented Jul 10, 2023

Well, at last, a pair of tests for the get tank operations. Honestly I'm a bit dissapointed with the test module, since I thought would be a little more realistic, but well, at least I hope this 2 tests can eventually be helpful.

Copy link
Owner

@Difegue Difegue left a comment

Choose a reason for hiding this comment

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

Nice progress! Thanks for taking the time to write tests as well, even if they're simple for now.

Here are some more suggestions from me.
The API seems good bar a few things, so I think you could also update the Documentation with the new endpoints? 🙏

lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Utils/Routing.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Controller/Api/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Show resolved Hide resolved
lib/LANraragi/Controller/Api/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Show resolved Hide resolved
lib/LANraragi/Utils/Routing.pm Outdated Show resolved Hide resolved
…t Tankoubon - Added remove archive from Tankoubon - Modified Routes to use tankoubons in plural - Moved Array_difference to Generic.pm - Changed decoded to include_full_data
@EfronC
Copy link
Contributor Author

EfronC commented Jul 22, 2023

In the most recent commit I tackled many of the Change requests mentioned:

  • I modified the Documentation to add Tankoubon API, but just to mention that since I wasn't able to preview what I was writting with Gitbook, I'm not sure if the syntax is correct. Also a few EPs might be outdated, since I was editing as I was fixing a few things from this commit, so the result may differ a little in maybe one or more cases.
  • I changed Get Tankoubon to include Pagination data, and decoded now is called include_full_data, and appends a new field called full_data, which now will contain the Archive Objects.
  • Pagination in Get Tankoubon and Get Tankoubon List now accepts page=-1 to return everything.
  • Added the Endpoint Remove Archive from Tankoubon.
  • Updated the routes for the Tankoubon API to have tankoubon in plural.
  • Array_difference is now in Utils::Generic.pm
  • Few aditional requested changes.

lib/LANraragi/Controller/Api/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Controller/Api/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Outdated Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Show resolved Hide resolved
lib/LANraragi/Model/Tankoubon.pm Show resolved Hide resolved
Copy link
Owner

@Difegue Difegue left a comment

Choose a reason for hiding this comment

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

Thanks again for the documentation, it looks good to me at a glance. We'll see what gitbook makes of it once merged, sadly there's no way to test it in PRs. 🤔

I've only got a few cleanup suggestions left + what seems to be a missing zadd in the edit function.

@EfronC
Copy link
Contributor Author

EfronC commented Jul 25, 2023

Ok, most recent one apply most of the cleanings requested 👍

@Difegue
Copy link
Owner

Difegue commented Jul 25, 2023

Alright, let's get this first part merged!
I think I've said it like 10 times already but thanks a lot for putting up with the whole thing, this really helps a lot. 💪

I won't really have time to work on the rest until october due to other projects/obligations, so if you want to proceed with the search index/reading progress integrations, feel free. No obligations tho ofc

@holopin-bot @EfronC give-him-the-burg

@holopin-bot
Copy link

holopin-bot bot commented Jul 25, 2023

Congratulations @EfronC, you just earned a badge! Here it is: https://holopin.io/claim/clkidd1yb310350fl4a2fjmu7r

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

@holopin-bot
Copy link

holopin-bot bot commented Jul 25, 2023

Congratulations @EfronC, you just earned a holobyte! Here it is: https://holopin.io/holobyte/clkidd7zp311960fl4jxm28vdl

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

@Difegue Difegue merged commit 26fc048 into Difegue:dev Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants