Page MenuHomeSoftware Heritage

save-code-now: Make extra inputs required
ClosedPublic

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

Details

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 jenkinsJenkins console · Jenkins
Build 33403: arc lint + arc unit

Event Timeline

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.

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
assets/src/bundles/save/index.js
65–67

i recalled it created issues for other types.

i'll check back, thx.

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.

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.

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

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.