Skip to content
Snippets Groups Projects

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

Closed api/add_forge_now: Allow to use requests list endpoint with datatables
generated-differential-D7344-source into generated-differential-D7344-target
2 unresolved threads
Closed Antoine Lambert requested to merge generated-differential-D7344-source into generated-differential-D7344-target
2 unresolved threads

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

Migrated from D7344 (view on Phabricator)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Software Heritage Continuous Integration for public repositories
    Software Heritage Continuous Integration for public repositories @swh-public-ci ·

    Build is green

    Patch application report for D7344 (id=26552)

    Could not rebase; Attempt merge onto cdcc1efd...

    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](https://forge.softwareheritage.org/T3989 'view original for T3989 on Phabricator')
    Related to [T3991](https://forge.softwareheritage.org/T3991 'view original for T3991 on Phabricator')

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](https://forge.softwareheritage.org/T4030 'view original for T4030 on Phabricator')

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](https://forge.softwareheritage.org/T4027 'view original for T4027 on Phabricator')

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](https://forge.softwareheritage.org/T4026 'view original for T4026 on Phabricator')

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](https://forge.softwareheritage.org/T3990 'view original for T3990 on Phabricator')

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.

  • Antoine R. Dumont
    Antoine R. Dumont @ardumont started a thread on the diff
swh/web/api/views/add_forge_now.py
290 if order_dir == "desc":
291 field_order = "-" + field_order
292 add_forge_requests = add_forge_requests.order_by(field_order)
293 else:
294 add_forge_requests = add_forge_requests.order_by("-id")
295
296 per_page = int(request.GET["length"])
297 page_num = int(request.GET["start"]) // per_page + 1
298
299 if search_value:
300 add_forge_requests = add_forge_requests.filter(
301 Q(forge_type__icontains=search_value)
302 | Q(forge_url__icontains=search_value)
303 | Q(status__icontains=search_value)
304 )
305 else:
  • Antoine R. Dumont
    Antoine R. Dumont @ardumont ·
    Maintainer

    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... ¯_(ツ)_/¯

  • Antoine Lambert
    Antoine Lambert @anlambert ·
    Author Maintainer

    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.

  • Please register or sign in to reply
  • Antoine R. Dumont
    Antoine R. Dumont @ardumont ·
    Maintainer

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

  • Antoine R. Dumont
    Antoine R. Dumont @ardumont ·
    Maintainer

    Merge request was accepted

  • Antoine R. Dumont approved this merge request

    approved this merge request

  • vlorentz
    vlorentz @vlorentz ·
    Maintainer

    Merge request was returned for changes

  • vlorentz
    vlorentz @vlorentz started a thread on the diff
    • swh/web/api/views/add_forge_now.py
    279 table_data: Dict[str, Any] = {}
    280 table_data["recordsTotal"] = add_forge_requests.count()
    281 table_data["draw"] = int(request.GET["draw"])
    282
    283 search_value = request.GET.get("search[value]")
    284
    285 column_order = request.GET.get("order[0][column]")
    286 field_order = request.GET.get(f"columns[{column_order}][name]")
    287 order_dir = request.GET.get("order[0][dir]")
    288
    289 if field_order:
    290 if order_dir == "desc":
    291 field_order = "-" + field_order
    292 add_forge_requests = add_forge_requests.order_by(field_order)
    293 else:
    294 add_forge_requests = add_forge_requests.order_by("-id")
  • Antoine R. Dumont
    Antoine R. Dumont @ardumont ·
    Maintainer

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

  • Antoine Lambert
    Antoine Lambert @anlambert ·
    Author Maintainer

    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.

  • Antoine Lambert
    Antoine Lambert @anlambert ·
    Author Maintainer

    Merge request was abandoned

  • Antoine Lambert closed

    closed

  • vlorentz
    vlorentz @vlorentz ·
    Maintainer

    the advantage is that requests will not be throttled

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

  • Antoine Lambert
    Antoine Lambert @anlambert ·
    Author Maintainer

    ! In !1069 (closed), @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
    vlorentz @vlorentz ·
    Maintainer

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

  • Antoine Lambert
    Antoine Lambert @anlambert ·
    Author Maintainer

    ! In !1069 (closed), @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
    vlorentz @vlorentz ·
    Maintainer

    meh

  • Antoine Lambert
    Antoine Lambert @anlambert ·
    Author Maintainer

    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.

    !1070 (closed)

    • Please register or sign in to reply