Create abaquery forums #3549

jonasdeluna · 2024-01-23T20:21:43Z

Create the abaquery forum backend.

linear · 2024-02-07T21:02:43Z

norbye · 2024-02-20T20:36:05Z

lego/api/v1.py

@@ -192,6 +194,7 @@
    r"surveys/(?P<survey_pk>\d+)/submissions", SubmissionViewSet, basename="submission"
 )
 router.register(r"tags", TagViewSet)
+router.register(r"threads", ThreadViewSet)


Was going to leave this comment on your lego-webapp PR, because I think it would simplify quite a bit of logic and over-fetching of information - but I'll leave it here as this is the source if the suggestion.

The way I understand the relationships between the models, the relationships are as follows:

A forum has 0-to-many threads, and a single thread has only one parent forum

A thread has 0-to-many comments, and a single comment has only one parent thread

If this is the case, have you considered changing the API URL structure to something like

/forums/ -> PublicForum[] with pagination

/forums/<id> -> DetailedForum

/forums/<id>/threads -> PublicThread[] with pagination

/forums/<id>/threads/<id> -> DetailedThread

/forums/<id>/threads/<id>/comments -> DetailedComment[] with pagination

With this structure, you could fetch data in the frontend and put it directly into the state, as it will already be in the format you want to use it (instead of having to filter through every thread to find the threads relevant to the one forum you are currently in).
It would also allow you to simply fetch the threads relevant to your current forum, instead of having to fetch all threads and filter them on the frontend - which imo is a bad practice and could lead to a lot of unnecessary fetching if there are lots of threads (for example, what would happen if you load the 10 first threads with the current implementation, then you filter them on the frontend, but it turns out all the first threads belonged to another forum - so you have to fetch another page of threads, not knowing if any of those are going to be relevant to the forum you actually want before you have filtered them in the frontend)

Okay thanks for the feedback I will look into this closer, this seems like a good suggestion

The threads only have a unique ID though which is not relative to the parent forum. Do you still think /forum/id/thread/id would make sense in this case?

Personally yeah, I think that would make sense - or you could change to slugs so it would be something like /forum/abahus/threads/kollektiv-paa-mollenberg, /forum/abahus/kollektiv-paa-mollenberg or /forum/abahus/threads/419.

There are many ways to design this though - I'm just suggesting this as I think it's one of the more popular designs and thus easier to understand right away. Plus it's closer to the actual use in the frontend requiring less filtering and logic there.

Although I'd guess that the others you have requested reviews from get this, or at least my first comment on email - so if there are other thoughts on this feel free to share(:

Okay, trying to add these but it seems we strictly use the router and not urlpatterns. Not sure if I'm allowed to change this and add some custom routes like this:
path('<int:forum_id>/threads/', ThreadViewSet.as_view({'get': 'list', 'post': 'create'}), name='forum-threads-list'),

I added it now but I think @ivarnakken will have some thoughts about this...

haha this is completely fine by me. I think @norbye's suggestion is good.

I'm not sure why you need this custom routing config though

I don't think using router.register supports custom patterns like this?
I've now updated it in backend and front-end.
Issue is still that action_grant is only send on list endpoint so either I need to pass it down or refetch list on every page which seems dumb...

Also, would an offset pagination make more sense in this case than pointer?

I don't think using router.register supports custom patterns like this?

Think you're right about that one, unless we go into the business of 3rd party extensions, which seems unnecessary.

Issue is still that action_grant is only send on list endpoint so either I need to pass it down or refetch list on every page which seems dumb...

I haven't looked into how we actually handle the action grants, but I know that /events sends action grants to each event - so I would assume thats not too complicated to do here as well.

Also, would an offset pagination make more sense in this case than pointer?

Personally I don't really mind either way - but either way I'd think that could be something for another PR down the line

codecov · 2024-02-24T19:13:49Z

Codecov Report

Attention: Patch coverage is 88.88889% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 88.24%. Comparing base (cc3afbf) to head (97b1b85).

❗ Current head 97b1b85 differs from pull request most recent head fd5913a. Consider uploading reports for the commit fd5913a to get more accurate results

Files	Patch %	Lines
lego/apps/forums/views.py	68.75%	20 Missing ⚠️
lego/apps/forums/permissions.py	73.07%	14 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #3549    +/-   ##
========================================
  Coverage   88.24%   88.24%            
========================================
  Files         670      678     +8     
  Lines       21089    21393   +304     
========================================
+ Hits        18609    18879   +270     
- Misses       2480     2514    +34

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jonasdeluna · 2024-02-24T19:26:04Z

@ivarnakken Could this be merged now that tests pass?

norbye

Been a while since I worked on anything in the backend - so take it with a pinch of salt - but looks good to me(:

norbye · 2024-02-24T21:45:54Z

lego/api/v1.py

@@ -192,6 +196,7 @@
    r"surveys/(?P<survey_pk>\d+)/submissions", SubmissionViewSet, basename="submission"
 )
 router.register(r"tags", TagViewSet)
+router.register(r"threads", ThreadViewSet)


Might want to delete this now that the other endpoint is implemented

norbye · 2024-02-24T21:51:24Z

lego/api/v1.py

@@ -192,6 +194,7 @@
    r"surveys/(?P<survey_pk>\d+)/submissions", SubmissionViewSet, basename="submission"
 )
 router.register(r"tags", TagViewSet)
+router.register(r"threads", ThreadViewSet)


I don't think using router.register supports custom patterns like this?

Think you're right about that one, unless we go into the business of 3rd party extensions, which seems unnecessary.

Issue is still that action_grant is only send on list endpoint so either I need to pass it down or refetch list on every page which seems dumb...

I haven't looked into how we actually handle the action grants, but I know that /events sends action grants to each event - so I would assume thats not too complicated to do here as well.

Also, would an offset pagination make more sense in this case than pointer?

Personally I don't really mind either way - but either way I'd think that could be something for another PR down the line

lego/apps/forums/migrations/0002_thread.py

jonasdeluna · 2024-02-26T16:36:41Z

@norbye Thanks for the review, however, I've resolved the article issue and I think I'll be merging this (assuming that's okay) just to get the MVP out.
Other comments will be fixed in the next sprint (with new features outlined on linear aswell)

jonasdeluna added backend new-feature Pull requests that introduce or issues that suggest a new feature labels Jan 23, 2024

jonasdeluna self-assigned this Jan 23, 2024

jonasdeluna requested review from ivarnakken and removed request for ivarnakken January 23, 2024 20:31

jonasdeluna marked this pull request as draft January 23, 2024 23:40

jonasdeluna marked this pull request as ready for review February 1, 2024 19:36

jonasdeluna requested review from danielyanghansen, ivarnakken, eikhr and PeterJFB February 1, 2024 19:36

jonasdeluna mentioned this pull request Feb 18, 2024

Create abaquery forum webkom/lego-webapp#4446

Merged

4 tasks

jonasdeluna force-pushed the create-abaquery-forums branch from f29c249 to fc764d1 Compare February 19, 2024 11:02

norbye reviewed Feb 20, 2024

View reviewed changes

jonasdeluna force-pushed the create-abaquery-forums branch 3 times, most recently from 0e7bd26 to e00b525 Compare February 23, 2024 15:28

Create the abaquery forums in backend

8a24356

jonasdeluna force-pushed the create-abaquery-forums branch 5 times, most recently from 787014a to cdd26f1 Compare February 24, 2024 19:11

norbye approved these changes Feb 24, 2024

View reviewed changes

jonasdeluna force-pushed the create-abaquery-forums branch 2 times, most recently from 97b1b85 to 36161fb Compare February 26, 2024 17:00

Fix type error on view

fd5913a

jonasdeluna force-pushed the create-abaquery-forums branch from 36161fb to fd5913a Compare February 26, 2024 17:04

jonasdeluna merged commit c48fb05 into master Feb 26, 2024
1 check passed

jonasdeluna deleted the create-abaquery-forums branch February 26, 2024 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create abaquery forums #3549

Create abaquery forums #3549

jonasdeluna commented Jan 23, 2024 •

edited

linear bot commented Feb 7, 2024

norbye Feb 20, 2024 •

edited

jonasdeluna Feb 20, 2024

jonasdeluna Feb 20, 2024

norbye Feb 21, 2024

jonasdeluna Feb 22, 2024 •

edited

ivarnakken Feb 22, 2024

ivarnakken Feb 22, 2024

jonasdeluna Feb 22, 2024 •

edited

norbye Feb 24, 2024

codecov bot commented Feb 24, 2024 •

edited

jonasdeluna commented Feb 24, 2024

norbye left a comment

norbye Feb 24, 2024

norbye Feb 24, 2024

jonasdeluna commented Feb 26, 2024

Create abaquery forums #3549

Create abaquery forums #3549

Conversation

jonasdeluna commented Jan 23, 2024 • edited

linear bot commented Feb 7, 2024

norbye Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasdeluna Feb 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasdeluna Feb 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 24, 2024 • edited

Codecov Report

jonasdeluna commented Feb 24, 2024

norbye left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasdeluna commented Feb 26, 2024

jonasdeluna commented Jan 23, 2024 •

edited

norbye Feb 20, 2024 •

edited

jonasdeluna Feb 22, 2024 •

edited

jonasdeluna Feb 22, 2024 •

edited

codecov bot commented Feb 24, 2024 •

edited