Page MenuHomeSoftware Heritage

Add feature flag to toggle on/off add-forge-now feature
ClosedPublic

Authored by ardumont on Mar 17 2022, 5:46 PM.

Details

Summary

That feature is deactivated by default. If this lands, we'll need to toggle it
on for the staging infra.

As an implementation detail, when toggled off, this hides the new add-forge-now menu links.
That also plainly deactivates the add-forge-now urls from being served by the server.

Related to T4040

Test Plan

tox, cypress happy (add-forge-now feature enabled for tests)

Diff Detail

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

Event Timeline

swh/web/templates/layout.html
236–238

somehow, if addforgenow_enabled and (user.is_staff or ...) was not understood in the template ¯\_(ツ)_/¯.

Build is green

Patch application report for D7370 (id=26651)

Could not rebase; Attempt merge onto 7bfbda85ff...

Merge made by the 'recursive' strategy.
 assets/src/bundles/add_forge/create-request.js     |  92 ++++
 assets/src/bundles/add_forge/index.js              |  11 +
 .../src/bundles/add_forge/moderation-dashboard.js  |  55 +++
 cypress/fixtures/add-forge-now-requests.json       |  79 ++++
 .../add-forge-now-request-create.spec.js           | 139 ++++++
 .../add-forge-now-requests-moderation.spec.js      | 122 ++++++
 cypress/integration/deposit-admin.spec.js          |  52 ++-
 cypress/support/index.js                           |   8 +
 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                     | 107 +++++
 swh/web/admin/add_forge_now.py                     |  30 ++
 swh/web/admin/urls.py                              |   3 +-
 swh/web/api/urls.py                                |   1 +
 swh/web/api/views/add_forge_now.py                 | 355 +++++++++++++++
 swh/web/auth/utils.py                              |   1 +
 swh/web/common/utils.py                            |   9 +-
 swh/web/config.py                                  |   9 +
 swh/web/inbound_email/__init__.py                  |   0
 swh/web/inbound_email/apps.py                      |  11 +
 .../management/commands/process_inbound_email.py   |  73 ++++
 swh/web/inbound_email/signals.py                   |  36 ++
 swh/web/inbound_email/utils.py                     |  63 +++
 swh/web/settings/common.py                         |   2 +
 swh/web/settings/tests.py                          |   1 +
 .../templates/add_forge_now/create-request.html    | 234 ++++++++++
 .../add_forge_now/requests-moderation.html         |  45 ++
 swh/web/templates/layout.html                      |  22 +-
 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/create_test_admin.py                 |  10 +-
 swh/web/tests/create_test_users.py                 |  23 +-
 .../tests/inbound_email/test_management_command.py | 161 +++++++
 swh/web/tests/inbound_email/test_utils.py          | 113 +++++
 swh/web/tests/test_config.py                       |  23 +
 swh/web/tests/utils.py                             |  14 +-
 swh/web/urls.py                                    |   1 +
 43 files changed, 2890 insertions(+), 19 deletions(-)
 create mode 100644 assets/src/bundles/add_forge/create-request.js
 create mode 100644 assets/src/bundles/add_forge/index.js
 create mode 100644 assets/src/bundles/add_forge/moderation-dashboard.js
 create mode 100644 cypress/fixtures/add-forge-now-requests.json
 create mode 100644 cypress/integration/add-forge-now-request-create.spec.js
 create mode 100644 cypress/integration/add-forge-now-requests-moderation.spec.js
 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/admin/add_forge_now.py
 create mode 100644 swh/web/api/views/add_forge_now.py
 create mode 100644 swh/web/inbound_email/__init__.py
 create mode 100644 swh/web/inbound_email/apps.py
 create mode 100644 swh/web/inbound_email/management/commands/process_inbound_email.py
 create mode 100644 swh/web/inbound_email/signals.py
 create mode 100644 swh/web/inbound_email/utils.py
 create mode 100644 swh/web/templates/add_forge_now/create-request.html
 create mode 100644 swh/web/templates/add_forge_now/requests-moderation.html
 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
 create mode 100644 swh/web/tests/inbound_email/test_management_command.py
 create mode 100644 swh/web/tests/inbound_email/test_utils.py
 create mode 100644 swh/web/tests/test_config.py
Changes applied before test
commit e6993b2ecd30970dcece69112af6eb2ce9d6ec01
Merge: 7bfbda85 72a4101f
Author: Jenkins user <jenkins@localhost>
Date:   Thu Mar 17 16:46:49 2022 +0000

    Merge branch 'diff-target' into HEAD

commit 72a4101f5ca955950c154799a7852ab92c51700d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Mar 17 17:41:44 2022 +0100

    Add feature flag to toggle on/off add-forge-now feature
    
    Related to T4040

commit 7e61894e90438f53c625961350d92cd01dc62138
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Mar 17 10:14:51 2022 +0100

    Add moderation view to list add-forge-now requests
    
    In a future commit, it will be possible to browse from that view to the detailed request
    dashboard.
    
    This also reworks the left menu to allow access to staff or people with the add forge
    now moderator permission.
    
    Related to T3991

commit 4e23aa2eb8fca1f5c62a745f44f2f4b3b46003a9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Mar 17 10:13:22 2022 +0100

    Fix access for people with permission access to deposit moderation
    
    This will allow easier adaptations when adding new views with different permissions.

commit 6c96f964adc7ffb7e9ea32de6d240a6a45bd8b97
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Mar 17 11:33:57 2022 +0100

    Create "Add forge now" request
    
    Added a form to submit an "add forge now" request.
    Left menus changed to include "add forge now" page.
    Possbility to browse existing requests is added
    in a seperate tab in the same view.
    Cypress tests
    
    Related to T3998
    Related to T3989
    Related to T4012
    Related to T3988

commit 80e55b0a12eb27a5df72acf6fe07640db12eaf27
Merge: c24e6822 9f2e1cb9
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Wed Mar 16 16:21:35 2022 +0100

    Merge branch 'master' into sprint-add-forge-now

commit c24e6822a1569817b6d970fbc550cdf66f2188f0
Merge: ca39b06c 627ba79a
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Wed Mar 16 14:20:52 2022 +0100

    Merge branch 'master' into sprint-add-forge-now

commit ca39b06c46ba42f73f898377d3093299ea21a85a
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Wed Mar 9 16:37:54 2022 +0100

    inbound_email: add utilities to extract and filter email recipients

commit bf099ce9654d0b765f0820189857ab7c7beb6c6b
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Wed Mar 9 14:29:37 2022 +0100

    Bootstrap a common app for management of inbound emails

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

I think we should really block the views as well as the links when the feature is not enabled.

For instance, the API endpoints would still be visible in the API documentation/index if the relevant views are still imported, so they would be really easily discoverable.

I think this would be doable by

  • guarding the import of swh.web.add_forge_now.views in swh.web.urls depending on the feature flag
  • guarding the import of swh.web.api.views.add_forge_now in swh.web.api.urls depending on the feature flag

(as a side note, I don't really understand why these views are split in two modules)

swh/web/common/utils.py
317

Would it make sense to just add a "FEATURES": config["features"] entry here? This would allow templates to just use {% if FEATURES.add_forge_now %} and to keep the exact same pattern for future features.

swh/web/config.py
129

Use underscores to make it easier to use values from the dict in templates?

I think we should really block the views as well as the links when the feature is not enabled.

yes, i agree, i was being lazy and wanted some validation from others ;)

For instance, the API endpoints would still be visible in the API documentation/index if the relevant views are still imported, so they would be really easily discoverable.

true, i had forgotten about the autodocumentation part.

I think this would be doable by

  • guarding the import of swh.web.add_forge_now.views in swh.web.urls depending on the feature flag
  • guarding the import of swh.web.api.views.add_forge_now in swh.web.api.urls depending on the feature flag

oh simple enough, thanks, good idea!
I'll check if i can do that tomorrow morning.

(as a side note, I don't really understand why these views are split in two modules)

It's currently consistently separated in "public" views and "admin" views.
I've just followed directives in previous diffs so ¯\_(ツ)_/¯.

swh/web/common/utils.py
317

yes, it'd make sense.
We may have to enforce the key to exist in all case (drop the ternary case i wrapped in is_feature_enabled function).

Build is green

Patch application report for D7370 (id=26655)

Could not rebase; Attempt merge onto 7bfbda85ff...

Merge made by the 'recursive' strategy.
 assets/src/bundles/add_forge/create-request.js     |  92 ++++
 assets/src/bundles/add_forge/index.js              |  11 +
 .../src/bundles/add_forge/moderation-dashboard.js  |  55 +++
 cypress/fixtures/add-forge-now-requests.json       |  79 ++++
 .../add-forge-now-request-create.spec.js           | 139 ++++++
 .../add-forge-now-requests-moderation.spec.js      | 122 ++++++
 cypress/integration/deposit-admin.spec.js          |  52 ++-
 cypress/support/index.js                           |   8 +
 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                     | 107 +++++
 swh/web/admin/add_forge_now.py                     |  30 ++
 swh/web/admin/urls.py                              |   6 +-
 swh/web/api/urls.py                                |   1 +
 swh/web/api/views/add_forge_now.py                 | 355 +++++++++++++++
 swh/web/auth/utils.py                              |   1 +
 swh/web/common/utils.py                            |   7 +-
 swh/web/config.py                                  |   9 +
 swh/web/inbound_email/__init__.py                  |   0
 swh/web/inbound_email/apps.py                      |  11 +
 .../management/commands/process_inbound_email.py   |  73 ++++
 swh/web/inbound_email/signals.py                   |  36 ++
 swh/web/inbound_email/utils.py                     |  63 +++
 swh/web/settings/common.py                         |   2 +
 swh/web/settings/tests.py                          |   1 +
 .../templates/add_forge_now/create-request.html    | 234 ++++++++++
 .../add_forge_now/requests-moderation.html         |  45 ++
 swh/web/templates/layout.html                      |  22 +-
 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/create_test_admin.py                 |  10 +-
 swh/web/tests/create_test_users.py                 |  23 +-
 .../tests/inbound_email/test_management_command.py | 161 +++++++
 swh/web/tests/inbound_email/test_utils.py          | 113 +++++
 swh/web/tests/test_config.py                       |  23 +
 swh/web/tests/utils.py                             |  14 +-
 swh/web/urls.py                                    |   7 +-
 43 files changed, 2896 insertions(+), 20 deletions(-)
 create mode 100644 assets/src/bundles/add_forge/create-request.js
 create mode 100644 assets/src/bundles/add_forge/index.js
 create mode 100644 assets/src/bundles/add_forge/moderation-dashboard.js
 create mode 100644 cypress/fixtures/add-forge-now-requests.json
 create mode 100644 cypress/integration/add-forge-now-request-create.spec.js
 create mode 100644 cypress/integration/add-forge-now-requests-moderation.spec.js
 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/admin/add_forge_now.py
 create mode 100644 swh/web/api/views/add_forge_now.py
 create mode 100644 swh/web/inbound_email/__init__.py
 create mode 100644 swh/web/inbound_email/apps.py
 create mode 100644 swh/web/inbound_email/management/commands/process_inbound_email.py
 create mode 100644 swh/web/inbound_email/signals.py
 create mode 100644 swh/web/inbound_email/utils.py
 create mode 100644 swh/web/templates/add_forge_now/create-request.html
 create mode 100644 swh/web/templates/add_forge_now/requests-moderation.html
 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
 create mode 100644 swh/web/tests/inbound_email/test_management_command.py
 create mode 100644 swh/web/tests/inbound_email/test_utils.py
 create mode 100644 swh/web/tests/test_config.py
Changes applied before test
commit 7394e394c8f72975eb9e0ba49c579b180721b3b3
Merge: 7bfbda85 b30a1203
Author: Jenkins user <jenkins@localhost>
Date:   Thu Mar 17 17:21:43 2022 +0000

    Merge branch 'diff-target' into HEAD

commit b30a120340bce5fe9f8ef9f1fabdf73f2d4e9917
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Mar 17 17:41:44 2022 +0100

    Add feature flag to toggle on/off add-forge-now feature
    
    This also prevents the server code from being served when the feature flag is off.
    
    Related to T4040

commit 7e61894e90438f53c625961350d92cd01dc62138
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Mar 17 10:14:51 2022 +0100

    Add moderation view to list add-forge-now requests
    
    In a future commit, it will be possible to browse from that view to the detailed request
    dashboard.
    
    This also reworks the left menu to allow access to staff or people with the add forge
    now moderator permission.
    
    Related to T3991

commit 4e23aa2eb8fca1f5c62a745f44f2f4b3b46003a9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Mar 17 10:13:22 2022 +0100

    Fix access for people with permission access to deposit moderation
    
    This will allow easier adaptations when adding new views with different permissions.

commit 6c96f964adc7ffb7e9ea32de6d240a6a45bd8b97
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Thu Mar 17 11:33:57 2022 +0100

    Create "Add forge now" request
    
    Added a form to submit an "add forge now" request.
    Left menus changed to include "add forge now" page.
    Possbility to browse existing requests is added
    in a seperate tab in the same view.
    Cypress tests
    
    Related to T3998
    Related to T3989
    Related to T4012
    Related to T3988

commit 80e55b0a12eb27a5df72acf6fe07640db12eaf27
Merge: c24e6822 9f2e1cb9
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Wed Mar 16 16:21:35 2022 +0100

    Merge branch 'master' into sprint-add-forge-now

commit c24e6822a1569817b6d970fbc550cdf66f2188f0
Merge: ca39b06c 627ba79a
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Wed Mar 16 14:20:52 2022 +0100

    Merge branch 'master' into sprint-add-forge-now

commit ca39b06c46ba42f73f898377d3093299ea21a85a
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Wed Mar 9 16:37:54 2022 +0100

    inbound_email: add utilities to extract and filter email recipients

commit bf099ce9654d0b765f0820189857ab7c7beb6c6b
Author: Nicolas Dandrimont <nicolas@dandrimont.eu>
Date:   Wed Mar 9 14:29:37 2022 +0100

    Bootstrap a common app for management of inbound emails

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

(as a side note, I don't really understand why these views are split in two modules)

It's currently consistently separated in "public" views and "admin" views.

Oh, I see.

anlambert added a subscriber: anlambert.

Looks good to me.

This revision is now accepted and ready to land.Mar 18 2022, 11:07 AM