Page MenuHomeSoftware Heritage
Differential D7344

api/add_forge_now: Allow to use requests list endpoint with datatables
AbandonedPublic
Actions

Authored by anlambert on Mar 14 2022, 3:48 PM.

Details

Reviewers
ardumont
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T3989: [Add forge now] Create "browse add requests" tab
T3991: [Add forge now] Create moderation dashboard
Commits
rDWAPPSf6343395310b: api/add_forge_now: Allow to use requests list endpoint with datatables
Summary

datatables is the javascript library we use on the frontend side to
display interactive tables.

As we use server-side processing, table data must be provided in a
paginated way by an HTTP endpoint.

Response format expected by datatables is different from the one
returned by the Web API endpoint listing add-forge requests.

So adapt the response format of that endpoint when we know the
input request has been sent by datatables.

Related to T3989
Related to T3991

Diff Detail

Repository
rDWAPPS Web applications
Branch
sprint-add-forge-now
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27432
Build 42930: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 42929: arc lint + arc unit

Event Timeline

anlambert created this revision.Mar 14 2022, 3:48 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptMar 14 2022, 3:48 PM
swh-public-ci added a comment.Mar 14 2022, 4:04 PM

Build is green

Patch application report for D7344 (id=26552)

Could not rebase; Attempt merge onto cdcc1efdfb...

Merge made by the 'recursive' strategy.
 swh/web/add_forge_now/__init__.py                |   0
 swh/web/add_forge_now/apps.py                    |  10 +
 swh/web/add_forge_now/migrations/0001_initial.py | 109 ++++
 swh/web/add_forge_now/migrations/__init__.py     |   0
 swh/web/add_forge_now/models.py                  |  99 ++++
 swh/web/add_forge_now/tests/test_migration.py    |  62 +++
 swh/web/api/urls.py                              |   1 +
 swh/web/api/views/add_forge_now.py               | 399 ++++++++++++++
 swh/web/settings/common.py                       |   1 +
 swh/web/tests/add_forge_now/test_models.py       |  26 +
 swh/web/tests/api/views/test_add_forge_now.py    | 633 +++++++++++++++++++++++
 swh/web/tests/utils.py                           |  10 +-
 12 files changed, 1347 insertions(+), 3 deletions(-)
 create mode 100644 swh/web/add_forge_now/__init__.py
 create mode 100644 swh/web/add_forge_now/apps.py
 create mode 100644 swh/web/add_forge_now/migrations/0001_initial.py
 create mode 100644 swh/web/add_forge_now/migrations/__init__.py
 create mode 100644 swh/web/add_forge_now/models.py
 create mode 100644 swh/web/add_forge_now/tests/test_migration.py
 create mode 100644 swh/web/api/views/add_forge_now.py
 create mode 100644 swh/web/tests/add_forge_now/test_models.py
 create mode 100644 swh/web/tests/api/views/test_add_forge_now.py
Changes applied before test
commit db062f03f63bd86022647c2a3280bf005a5e35e9
Merge: cdcc1efd f6343395
Author: Jenkins user <jenkins@localhost>
Date:   Mon Mar 14 14:49:34 2022 +0000

    Merge branch 'diff-target' into HEAD

commit f6343395310bc226a50a283c88304006e8ece337
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Mon Mar 14 15:35:17 2022 +0100

    api/add_forge_now: Allow to use requests list endpoint with datatables
    
    datatables is the javascript library we use on the frontend side to
    display interactive tables.
    
    As we use server-side processing, table data must be provided in a
    paginated way by an HTTP endpoint.
    
    Response format expected by datatables is different from the one
    returned by the Web API endpoint listing add-forge requests.
    
    So adapt the response format of that endpoint when we know the
    input request has been sent by datatables.
    
    Related to T3989
    Related to T3991

commit 26748e56ecce5877fce3a215eba06a19ebb8342f
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Mar 10 14:09:36 2022 +0100

    api: Add endpoint to get details about an add-forge request
    
    Related to T4030

commit 294a95c711bc40622c6d718afa2535f377b90dfc
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Mar 9 16:30:06 2022 +0100

    api: Add endpoint to list add-forge requests
    
    Related to T4027

commit 1bf17d6a75fd7265b1da9c9e87220efa62869c93
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Mar 9 16:28:03 2022 +0100

    api: Add endpoint to update an add-forge request
    
    Related to T4026

commit 130e9faa9bcab18bc3c76dc898546edb89f3f9b8
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Mar 8 15:23:50 2022 +0100

    api: Add endpoint to create an add-forge request
    
    Related to T3990

commit 03101208803501e5178f35d18d171e740ee4ca76
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Mar 8 11:34:44 2022 +0100

    add_forge_now: Bootstrap app and model

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1454/ for more details.

Harbormaster completed remote builds in B27432: Diff 26552.Mar 14 2022, 4:04 PM
anlambert requested review of this revision.Mar 14 2022, 4:04 PM
ardumont added a subscriber: ardumont.Mar 14 2022, 5:07 PM
ardumont added inline comments.
swh/web/api/views/add_forge_now.py
305

I recall having read that kind of code for other view using datatables (deposit for one).
Wondering whether we should open dedicated api views for the datatable views (which calls the main list api unchanged) instead...
That sounds convoluted though...

But the if... else conditional in the api here for datatable is not so smooth either... ¯\_(ツ)_/¯

ardumont accepted this revision.Mar 14 2022, 5:10 PM

One remark inline but short of having a better solution, let's land this.

This revision is now accepted and ready to land.Mar 14 2022, 5:10 PM
anlambert added inline comments.Mar 14 2022, 5:27 PM
swh/web/api/views/add_forge_now.py
305

Yes this is a quick and dirty solution.

Other approach would be to create a new dedicated listing endpoint to be used by datatables only,
this is what it is done for save code now for instance. But it is at the cost of some code duplication
with the Web API endpoint to list the add-forge requests.

vlorentz requested changes to this revision.Mar 14 2022, 5:41 PM
vlorentz added a subscriber: vlorentz.
vlorentz added inline comments.
swh/web/api/views/add_forge_now.py
305

why keep the non-datatables version at all? can't regular API users use a subset of queries used by datatables?

This revision now requires changes to proceed.Mar 14 2022, 5:41 PM
vlorentz added inline comments.Mar 14 2022, 5:46 PM
swh/web/api/views/add_forge_now.py
294

missing handling for order_dir

ardumont added a comment.Mar 14 2022, 6:59 PM

jsyk, it's pushed in sprint-add-forge-now branch.

anlambert requested review of this revision.Mar 15 2022, 11:13 AM
anlambert marked an inline comment as done.
anlambert added inline comments.
swh/web/api/views/add_forge_now.py
294

In practice, that execution path will not be taken as datatables will always send query parameters about which column to use for sorting, this was just to add a fallback.

305

Datatables request are not really straightforward to use for a standard user, see for instance that request to display the first page of save code now requests:

https://archive.softwareheritage.org/save/requests/list/all/?draw=3&columns%5B0%5D%5Bdata%5D=save_request_date&columns%5B0%5D%5Bname%5D=request_date&columns%5B0%5D%5Bsearchable%5D=true&columns%5B0%5D%5Borderable%5D=true&columns%5B0%5D%5Bsearch%5D%5Bvalue%5D=&columns%5B0%5D%5Bsearch%5D%5Bregex%5D=false&columns%5B1%5D%5Bdata%5D=visit_type&columns%5B1%5D%5Bname%5D=visit_type&columns%5B1%5D%5Bsearchable%5D=true&columns%5B1%5D%5Borderable%5D=true&columns%5B1%5D%5Bsearch%5D%5Bvalue%5D=&columns%5B1%5D%5Bsearch%5D%5Bregex%5D=false&columns%5B2%5D%5Bdata%5D=origin_url&columns%5B2%5D%5Bname%5D=origin_url&columns%5B2%5D%5Bsearchable%5D=true&columns%5B2%5D%5Borderable%5D=true&columns%5B2%5D%5Bsearch%5D%5Bvalue%5D=&columns%5B2%5D%5Bsearch%5D%5Bregex%5D=false&columns%5B3%5D%5Bdata%5D=save_request_status&columns%5B3%5D%5Bname%5D=status&columns%5B3%5D%5Bsearchable%5D=true&columns%5B3%5D%5Borderable%5D=true&columns%5B3%5D%5Bsearch%5D%5Bvalue%5D=&columns%5B3%5D%5Bsearch%5D%5Bregex%5D=false&columns%5B4%5D%5Bdata%5D=save_task_status&columns%5B4%5D%5Bname%5D=loading_task_status&columns%5B4%5D%5Bsearchable%5D=true&columns%5B4%5D%5Borderable%5D=true&columns%5B4%5D%5Bsearch%5D%5Bvalue%5D=&columns%5B4%5D%5Bsearch%5D%5Bregex%5D=false&columns%5B5%5D%5Bdata%5D=5&columns%5B5%5D%5Bname%5D=info&columns%5B5%5D%5Bsearchable%5D=true&columns%5B5%5D%5Borderable%5D=true&columns%5B5%5D%5Bsearch%5D%5Bvalue%5D=&columns%5B5%5D%5Bsearch%5D%5Bregex%5D=false&columns%5B6%5D%5Bdata%5D=6&columns%5B6%5D%5Bname%5D=&columns%5B6%5D%5Bsearchable%5D=true&columns%5B6%5D%5Borderable%5D=true&columns%5B6%5D%5Bsearch%5D%5Bvalue%5D=&columns%5B6%5D%5Bsearch%5D%5Bregex%5D=false&order%5B0%5D%5Bcolumn%5D=0&order%5B0%5D%5Bdir%5D=desc&start=0&length=10&search%5Bvalue%5D=&search%5Bregex%5D=false&_=1647338575022

So standard user should use the Web API endpoint as documented, the response adaptation for datatables is not documented anyway.

vlorentz added inline comments.Mar 15 2022, 11:15 AM
swh/web/api/views/add_forge_now.py
294

until it doesn't, and we will have a bug that will be hard to debug

305

ok

anlambert abandoned this revision.Mar 15 2022, 1:46 PM
anlambert marked an inline comment as done.

I am abandoning this and will submit a new diff where I will separate the datatables listing endpoint from the Web API one (the advantage is that requests will not be throttled) and handle the remaining comments from @vlorentz.

vlorentz added a comment.Mar 15 2022, 1:49 PM

the advantage is that requests will not be throttled

Why throttle the public API endpoint but not the datatables endpoint?

anlambert added a comment.Mar 15 2022, 2:07 PM
In D7344#191125, @vlorentz wrote:

the advantage is that requests will not be throttled

Why throttle the public API endpoint but not the datatables endpoint?

All API endpoints are throttled by default, we can increase the number of allowed requests per endpoint though.

For the datatables one, there is no reason to throttle it as it is only for displaying requests data and all users will be able to see the submitted add-forge requests (as with the save code now ones).

vlorentz added a comment.EditedMar 15 2022, 2:20 PM

The reason to throttle it is to avoid resource abuse; which is not limited to the API

anlambert added a comment.Mar 15 2022, 2:27 PM
In D7344#191127, @vlorentz wrote:

The reason to throttle it is to avoid resource abuse; which is not limited to the API

Currently it is, only API endpoints are throttled in swh-web.

vlorentz added a comment.Mar 15 2022, 3:06 PM

meh

anlambert added a comment.EditedMar 15 2022, 3:14 PM

I am abandoning this and will submit a new diff where I will separate the datatables listing endpoint from the Web API one (the advantage is that requests will not be throttled) and handle the remaining comments from @vlorentz.

D7351

anlambert added a commit: rDWAPPSf6343395310b: api/add_forge_now: Allow to use requests list endpoint with datatables.Mar 16 2022, 2:18 PM

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
web/
api/
views/
91 lines98%
Loading...
tests/
api/
views/
144 lines100%
Loading...

Diff 26552

View Options

swh/web/api/views/add_forge_now.py

Loading...
View Options

swh/web/tests/api/views/test_add_forge_now.py

Loading...
About · Developer information · Legal