Differential D5738

save-code-now: Make extra inputs required
ClosedPublic
Actions

Authored by ardumont on May 11 2021, 2:52 PM.

Details

Reviewers

anlambert

Group Reviewers

Reviewers

Maniphest Tasks

T3213: Enable save code now of software source code archives for specific users

Commits

rDWAPPS8157fd1c351a: archive: Make extra 'archives' input mandatory when visible

Summary

This also drops the filename which is not required since 1. it's part of the url and 2. the
loader knows how to deal without it.

The version is still part of inputs as it's mandatory from the loader's side.

Related to T3213

Test Plan

tox, cypress

Diff Detail

Repository

rDWAPPS Web applications

Branch

adapt-inputs

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 21501
Build 33404: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 33403: arc lint + arc unit

Event Timeline

ardumont created this revision.May 11 2021, 2:52 PM

ardumont mentioned this in D5676: Allow privileged user to trigger save code now bundle visit type.May 11 2021, 2:53 PM

Build is green

Patch application report for D5738 (id=20501)

Rebasing onto 7dac28063e...

Current branch diff-target is up to date.

Changes applied before test

commit 343b07f16def67a14e488fdc75fd2da5dd649fad
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue May 11 14:45:40 2021 +0200

    save-code-now: Display a small help below the extra inputs
    
    Related to T3213

commit a4c62d475c63503893dbc1149ddbaa39204cca87
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue May 11 14:43:28 2021 +0200

    archive: Make extra 'archives' input mandatory when visible
    
    Related to T3213

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

Harbormaster completed remote builds in B21475: Diff 20501.May 11 2021, 3:01 PM

ardumont requested review of this revision.May 11 2021, 3:01 PM

anlambert requested changes to this revision.May 11 2021, 3:26 PM

anlambert added inline comments.

assets/src/bundles/save/index.js
65–67	You need to add the required attribute directly in the html template instead <input type="text" class="form-control" id="swh-input-artifact-url" aria-describedby="artifactUrlHelp" required> ... <input type="text" class="form-control" id="swh-input-artifact-version" aria-describedby="artifactVersionHelp" required>
swh/web/templates/misc/origin-save.html
58	Looking at the displayed result, the help texts are quite redundant with the input titles so you can drop them after all I think. Instead, you should add a text feedback when the required inputs are empty. Just add the following under an input to do so: <div class="invalid-feedback">The artifact url is mandatory</div> You will get this when the form can not be validated.

This revision now requires changes to proceed.May 11 2021, 3:26 PM

ardumont added inline comments.May 11 2021, 3:35 PM

assets/src/bundles/save/index.js
65–67	i recalled it created issues for other types. i'll check back, thx.

ardumont added inline comments.May 11 2021, 3:56 PM

assets/src/bundles/save/index.js
65–67	Yes, i did the following because for the other type, with what you suggested (my first implementation btw), the submission hang. AFAIUI, it's because the required but not displayed inputs (for the 'archives' type) are invalid because empty. So the form submission actually does not occur because it's stopped for invalid inputs. This implementation which works sounds simple enough. Some website mentions the attribute 'disabled' to ignore the inputs we don't want. That would touch the same code.

ardumont added inline comments.May 11 2021, 4:01 PM

assets/src/bundles/save/index.js
65–67	Some website mentions the attribute 'disabled' to ignore the inputs we don't want. That would touch the same code. as discussed, I'll check if the following would work instead ;)

ardumont retitled this revision from save-code-now: Make extra inputs required and add some help message to save-code-now: Make extra inputs required.May 11 2021, 4:55 PM

ardumont edited the summary of this revision. (Show Details)

Use required/disabled stanza
Add explanatory message when the mandatory input is not provided
Drop the redundant help message (and the commit which came along)

Build is green

Patch application report for D5738 (id=20523)

Rebasing onto c1de981dcd...

First, rewinding head to replay your work on top of it...
Applying: archive: Make extra 'archives' input mandatory when visible

Changes applied before test

commit 183f6f6a31b152d17c191984996a402b1f74cf5f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue May 11 14:43:28 2021 +0200

    archive: Make extra 'archives' input mandatory when visible
    
    Related to T3213

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

Harbormaster completed remote builds in B21500: Diff 20523.May 11 2021, 5:20 PM

Adapted.

Looks good to me.

This revision is now accepted and ready to land.May 11 2021, 5:27 PM

Rebase

Build is green

Patch application report for D5738 (id=20524)

Rebasing onto c1de981dcd...

Current branch diff-target is up to date.

Changes applied before test

commit 8157fd1c351a3f8356da1e642d20000d2b73e66e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue May 11 14:43:28 2021 +0200

    archive: Make extra 'archives' input mandatory when visible
    
    Related to T3213

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

Harbormaster completed remote builds in B21501: Diff 20524.May 11 2021, 5:38 PM

Closed by commit rDWAPPS8157fd1c351a: archive: Make extra 'archives' input mandatory when visible (authored by ardumont). · Explain WhyMay 11 2021, 5:40 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDWAPPS8157fd1c351a: archive: Make extra 'archives' input mandatory when visible.