Page MenuHomeSoftware Heritage
Differential D7351

add_forge_now: Move datatables listing in dedicated endpoint
ClosedPublic
Actions

Authored by anlambert on Mar 15 2022, 3:12 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Maniphest Tasks
T3989: [Add forge now] Create "browse add requests" tab
T3991: [Add forge now] Create moderation dashboard
Commits
rDWAPPSb6d540d2e5fa: add_forge_now: Move datatables listing in dedicated endpoint
Summary

Merging datatables special processing in /api/1/add-forge/request/list
endpoint implementation was quite disturbing.

So better using a dedicated endpoint for that in a similar manner as
with save code now requests listing.

The endpoint to use with datatables is now the following one:
/add-forge/request/list/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 27450
Build 42953: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 42952: arc lint + arc unit

Event Timeline

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

Build has FAILED

Patch application report for D7351 (id=26570)

Could not rebase; Attempt merge onto 5bc77709cb...

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/add_forge_now/views.py                   |  87 ++++
 swh/web/api/urls.py                              |   1 +
 swh/web/api/views/add_forge_now.py               | 355 +++++++++++++++++
 swh/web/auth/utils.py                            |   1 +
 swh/web/settings/common.py                       |   1 +
 swh/web/tests/add_forge_now/test_models.py       |  26 ++
 swh/web/tests/add_forge_now/test_views.py        | 203 ++++++++++
 swh/web/tests/api/views/test_add_forge_now.py    | 483 +++++++++++++++++++++++
 swh/web/tests/conftest.py                        |  12 +-
 swh/web/tests/utils.py                           |  10 +-
 swh/web/urls.py                                  |   1 +
 17 files changed, 1456 insertions(+), 4 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/add_forge_now/views.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/add_forge_now/test_views.py
 create mode 100644 swh/web/tests/api/views/test_add_forge_now.py
Changes applied before test
commit a9e56da884c807a9e5aa698857ac617086231651
Merge: 5bc77709 b6d540d2
Author: Jenkins user <jenkins@localhost>
Date:   Tue Mar 15 14:13:03 2022 +0000

    Merge branch 'diff-target' into HEAD

commit b6d540d2e5fafe04eb71d3b1d8f5c194dafcbb62
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Mar 15 15:06:10 2022 +0100

    add_forge_now: Move datatables listing in dedicated endpoint
    
    Merging datatables special processing in /api/1/add-forge/request/list
    endpoint implementation was quite disturbing.
    
    So better using a dedicated endpoint for that in a similar manner as
    with save code now requests listing.
    
    The endpoint to use with datatables is now the following one:
    /add-forge/request/list/datatables.
    
    Related to T3989
    Related to T3991

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

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1456/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1456/console

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 15 2022, 3:14 PM
Harbormaster failed remote builds in B27450: Diff 26570!
anlambert mentioned this in D7344: api/add_forge_now: Allow to use requests list endpoint with datatables.Mar 15 2022, 3:15 PM
swh-public-ci added a comment.Mar 15 2022, 3:32 PM

Build is green

Patch application report for D7351 (id=26570)

Could not rebase; Attempt merge onto 5bc77709cb...

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/add_forge_now/views.py                   |  87 ++++
 swh/web/api/urls.py                              |   1 +
 swh/web/api/views/add_forge_now.py               | 355 +++++++++++++++++
 swh/web/auth/utils.py                            |   1 +
 swh/web/settings/common.py                       |   1 +
 swh/web/tests/add_forge_now/test_models.py       |  26 ++
 swh/web/tests/add_forge_now/test_views.py        | 203 ++++++++++
 swh/web/tests/api/views/test_add_forge_now.py    | 483 +++++++++++++++++++++++
 swh/web/tests/conftest.py                        |  12 +-
 swh/web/tests/utils.py                           |  10 +-
 swh/web/urls.py                                  |   1 +
 17 files changed, 1456 insertions(+), 4 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/add_forge_now/views.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/add_forge_now/test_views.py
 create mode 100644 swh/web/tests/api/views/test_add_forge_now.py
Changes applied before test
commit 235dde853c826e8d1b6fb3f11abc07b717f6fe09
Merge: 5bc77709 b6d540d2
Author: Jenkins user <jenkins@localhost>
Date:   Tue Mar 15 14:16:42 2022 +0000

    Merge branch 'diff-target' into HEAD

commit b6d540d2e5fafe04eb71d3b1d8f5c194dafcbb62
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Mar 15 15:06:10 2022 +0100

    add_forge_now: Move datatables listing in dedicated endpoint
    
    Merging datatables special processing in /api/1/add-forge/request/list
    endpoint implementation was quite disturbing.
    
    So better using a dedicated endpoint for that in a similar manner as
    with save code now requests listing.
    
    The endpoint to use with datatables is now the following one:
    /add-forge/request/list/datatables.
    
    Related to T3989
    Related to T3991

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/1457/ for more details.

Harbormaster completed remote builds in B27450: Diff 26570.Mar 15 2022, 3:32 PM
anlambert requested review of this revision.Mar 15 2022, 3:32 PM
ardumont added subscribers: jayeshv, ardumont.Mar 15 2022, 3:46 PM

@jayeshv heads up, ^ that means:

  1. @anlambert will have to force
  2. we will have to reset --hard the sprint-add-forge-now branch
  3. rebase sprint-add-forge-now-moderation-view branch on top of it (dropping the old top commit of that branch) again
swh/web/api/views/add_forge_now.py
197

i have a diff naming this SWH_MODERATOR_PERMISSION (matching the convention naming we have for SWH_AMBASSADOR_PERMISSION.
(I'm also moving that perm to the swh.auth.utils module so great it's done before my diff).

Any thoughts on the naming?

(I gathered moderator will eventually have more to do than just the add-forge now at some point).

ardumont accepted this revision.Mar 15 2022, 3:47 PM

lgtm

(i've only skimmed through the tests though)

This revision is now accepted and ready to land.Mar 15 2022, 3:47 PM
anlambert added a comment.Mar 15 2022, 3:53 PM

@anlambert will have to force

Nope, it's a new commit in the sprint-add-forge-now branch, no need to force push.

we will have to reset --hard the sprint-add-forge-now branch

You just need to pull the branch once that commit landed and rebase your feature branch on top of it.

swh/web/api/views/add_forge_now.py
197

It is better to have one permission for a specific feature, if we need a new moderator role for another one we will create a new permission.

ardumont added a comment.Mar 15 2022, 3:56 PM
In D7351#191167, @anlambert wrote:

@anlambert will have to force

Nope, it's a new commit in the sprint-add-forge-now branch, no need to force push.

ah yes, good.

we will have to reset --hard the sprint-add-forge-now branch

You just need to pull the branch once that commit landed and rebase your feature branch on top of it.

Yes but then after that we need to force push that branch anyway.
For now, we are incrementally progressing to add the missing coverage on the views.
We should never have started mixing multiple views in commits but it's done that way now...
So there fight.

Thanks for improving this.

anlambert closed this revision.Mar 15 2022, 3:57 PM

Landed in b6d540d2e5fa.

anlambert added a comment.Mar 15 2022, 4:02 PM

Yes but then after that we need to force push that branch anyway.
For now, we are incrementally progressing to add the missing coverage on the views.
We should never have started mixing multiple views in commits but it's done that way now...
So there fight.

I guess you can continue to work that way on the sprint-add-forge-now-moderation-view branch
and submit diffs with a view implementation + related tests once you converged to something stable.

Thanks for improving this.

Yes mixing datatables special processing in Web API endpoint implementation was a silly idea.
Save code now and mailmaps use a dedicated endpoint for datatables so better do the same here.

ardumont added a comment.EditedMar 15 2022, 4:59 PM
In D7351#191173, @anlambert wrote:

Yes but then after that we need to force push that branch anyway.
For now, we are incrementally progressing to add the missing coverage on the views.
We should never have started mixing multiple views in commits but it's done that way now...
So there fight.

I guess you can continue to work that way on the sprint-add-forge-now-moderation-view branch
and submit diffs with a view implementation + related tests once you converged to something stable.

yes, that's what i want to do but that's revealing challenging.
I'm currently on the rebase that branch and that's not so easy, this diff included a new views.py module which we already created on the moderation view branch...
hence some more fight...

anlambert added a commit: rDWAPPSb6d540d2e5fa: add_forge_now: Move datatables listing in dedicated endpoint.Mar 16 2022, 2:18 PM

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
web/
add_forge_now/
87 lines100%
Loading...
api/
views/
96 lines98%
Loading...
auth/
1 line100%
Loading...
tests/
add_forge_now/
203 lines100%
Loading...
api/
views/
224 lines100%
Loading...
12 lines94%
Loading...
1 line88%
Loading...

Diff 26570

View Options

swh/web/add_forge_now/views.py

Loading...
View Options

swh/web/api/views/add_forge_now.py

Loading...
View Options

swh/web/auth/utils.py

Loading...
View Options

swh/web/tests/add_forge_now/test_views.py

Loading...
View Options

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

Loading...
View Options

swh/web/tests/conftest.py

Loading...
View Options

swh/web/urls.py

Loading...
About · Developer information · Legal