Page MenuHomeSoftware Heritage
Differential D7859

fix mailto encoding for inbound email
ClosedPublic
Actions

Authored by bchauvet on May 19 2022, 9:22 AM.

Details

Reviewers
douardda
Group Reviewers
Reviewers
Maniphest Tasks
T4260: [add forge now] inbound email encoding
Commits
rDWAPPS7721b9b6a7a1: Fix mailto encoding for inbound email
Summary

Resolves T4260

Diff Detail

Repository
rDWAPPS Web applications
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 29475
Build 46057: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 46056: arc lint + arc unit

Unit TestsFailed
View All

TimeTest
0 msJenkins > "before all" hook for "should load add forge request details"::Tests / Cypress tests / Run cypress tests / Test add forge now request dashboard load "before all" hook for "should load add forge request details"
CypressError: `cy.request()` failed on: http://localhost:5004/api/1/add-forge/request/create/
972 msJenkins > "before each" hook for "should submit correct details"::Tests / Cypress tests / Run cypress tests / Test add forge now request update "before each" hook for "should submit correct details"
CypressError: `cy.request()` failed on: http://localhost:5004/api/1/add-forge/request/create/
666 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.add_forge_now.test_migration::Tests / Python tests / test_add_forge_now_allow_no_comment
460 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.add_forge_now.test_migration::Tests / Python tests / test_add_forge_now_initial_migration
465 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.web.tests.add_forge_now.test_migration::Tests / Python tests / test_add_forge_now_store_submitter_forward_username
View Full Test Results (2 Failed · 996 Passed · 5 Skipped)

Event Timeline

bchauvet created this revision.May 19 2022, 9:22 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptMay 19 2022, 9:23 AM
bchauvet added a task: T4260: [add forge now] inbound email encoding.May 19 2022, 9:24 AM
swh-public-ci added a comment.May 19 2022, 9:45 AM

Build is green

Patch application report for D7859 (id=28370)

Rebasing onto b548d2f261...

Current branch diff-target is up to date.
Changes applied before test
commit ffd8a035e2cbf12f109ab3af45828bc31fc575d7
Author: Benoit Chauvet <contact@benoitchauvet.com>
Date:   Thu May 19 09:20:36 2022 +0200

    fix mailto econding for inboud email

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

Harbormaster completed remote builds in B29468: Diff 28370.May 19 2022, 9:45 AM
bchauvet requested review of this revision.May 19 2022, 9:45 AM
jayeshv added a subscriber: jayeshv.May 19 2022, 10:02 AM
jayeshv added inline comments.
assets/src/bundles/add_forge/request-dashboard.js
59–60

Isn't this needed in the previous line (59) as well?

bchauvet updated this revision to Diff 28373.May 19 2022, 10:30 AM

UrlEncoded all email addresses

vlorentz added a subscriber: vlorentz.May 19 2022, 10:51 AM

good catch!

assets/src/bundles/add_forge/request-dashboard.js
136

IMO, URL-encoding should be here, where we actually build the URL.

text + emailTo and emailCc aren't necessarily meant only to be used for building URLs

ardumont added a subscriber: ardumont.May 19 2022, 10:55 AM

*thumbs up*

Beware your commit (and as a result your diff description) though.

It should be:
Fix mailto e*n*co**ding for inbo*u*nd email

And please add the Related to T4260 in the diff description (not in the commit message or the diff title).

Cheers,

swh-public-ci added a comment.May 19 2022, 10:56 AM

Build has FAILED

Patch application report for D7859 (id=28373)

Rebasing onto b548d2f261...

Current branch diff-target is up to date.
Changes applied before test
commit 6404f5784608a304d0f53677122aac458398c117
Author: Benoit Chauvet <contact@benoitchauvet.com>
Date:   Thu May 19 09:20:36 2022 +0200

    fix mailto econding for inboud email

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

Harbormaster failed remote builds in B29471: Diff 28373!May 19 2022, 10:56 AM
bchauvet updated this revision to Diff 28379.May 19 2022, 11:10 AM

fixed cypress tests

swh-public-ci added a comment.May 19 2022, 11:33 AM

Build has FAILED

Patch application report for D7859 (id=28379)

Rebasing onto 12661f9b3f...

First, rewinding head to replay your work on top of it...
Applying: fix mailto econding for inboud email
Changes applied before test
commit 8905966ed768b66beb0caa8f45a8e2c69f89d8b4
Author: Benoit Chauvet <contact@benoitchauvet.com>
Date:   Thu May 19 09:20:36 2022 +0200

    fix mailto econding for inboud email

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

Harbormaster failed remote builds in B29475: Diff 28379!May 19 2022, 11:33 AM
bchauvet updated this revision to Diff 28383.EditedMay 19 2022, 11:41 AM

URL-encording done directly on the url construction of the mailto

Related to T4260

bchauvet retitled this revision from fix mailto econding for inboud email (T4260) to fix mailto encoding for inbound email (T4260).May 19 2022, 11:44 AM
vlorentz added inline comments.May 19 2022, 11:57 AM
assets/src/bundles/add_forge/request-dashboard.js
136

now the code escapes newlines in emailText twice. Replace .replace(/\n/g, '%0D%0A') with .replace(/\n/g, '\r\n'), and encodeURIComponent will then turn '\r\n' into '%0D%0A'.

(And we should probably have tests to catch this)

swh-public-ci added a comment.May 19 2022, 12:06 PM

Build is green

Patch application report for D7859 (id=28383)

Rebasing onto 12661f9b3f...

First, rewinding head to replay your work on top of it...
Applying: fix mailto encoding for inbound email
Changes applied before test
commit 64a05da18a3dbbe18cbd99246ea2e0b5c7bfb4b0
Author: Benoit Chauvet <contact@benoitchauvet.com>
Date:   Thu May 19 09:20:36 2022 +0200

    fix mailto encoding for inbound email

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

Harbormaster completed remote builds in B29479: Diff 28383.May 19 2022, 12:06 PM
vlorentz retitled this revision from fix mailto encoding for inbound email (T4260) to fix mailto encoding for inbound email.May 19 2022, 12:10 PM
vlorentz edited the summary of this revision. (Show Details)
bchauvet updated this revision to Diff 28392.May 19 2022, 2:45 PM

fixed newline replacement

swh-public-ci added a comment.May 19 2022, 3:08 PM

Build is green

Patch application report for D7859 (id=28392)

Rebasing onto 12661f9b3f...

First, rewinding head to replay your work on top of it...
Applying: fix mailto encoding for inbound email
Changes applied before test
commit 78020b90a34d52ec4f57c357a693457bd1591af2
Author: Benoit Chauvet <contact@benoitchauvet.com>
Date:   Thu May 19 09:20:36 2022 +0200

    fix mailto encoding for inbound email

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

Harbormaster completed remote builds in B29486: Diff 28392.May 19 2022, 3:09 PM
vlorentz added a comment.May 23 2022, 3:28 PM

@bchauvet you pushed 7721b9b6a7a1 to master, but this diff's commit was fba125818391; so you need to close it manually

bchauvet marked 2 inline comments as done.May 24 2022, 11:51 AM
douardda accepted this revision.May 24 2022, 11:51 AM
This revision is now accepted and ready to land.May 24 2022, 11:51 AM
bchauvet closed this revision.May 24 2022, 11:52 AM
bchauvet added a commit: rDWAPPS7721b9b6a7a1: Fix mailto encoding for inbound email.

Revision Contents
Changeset List

PathSize
assets/
src/
bundles/
add_forge/
6 lines
cypress/
integration/
2 lines

Diff 28379

View Options

assets/src/bundles/add_forge/request-dashboard.js

Loading...
View Options

cypress/integration/add-forge-now-request-dashboard.spec.js

Loading...
About · Developer information · Legal