Error messages in the static theme submission wizard should be displayed next to the field that generated them

AlexandraMoga commented 5 years ago

STR:

Log in to Developer Hub
Submit a new static theme using the wizard
Enter some spam content in the "Name" field in order to trigger an error
Click on Finish Theme after completing all the submission steps
Check the error message received

Actual result: The error message is displayed at the bottom of the page

Expected result: The error message should be displayed right after the Name field to better emphasize what generated the error

Notes:

the same idea is relevant for other error messages that might be triggered in the submission wizard (see 2nd screenshot as example)
reproduced on all AMO servers with FF62, WIn10x64

[1]

[2]

eviljeff commented 5 years ago

this depends on mozilla/addons#6046

eviljeff commented 5 years ago

for [1] mozilla/addons#6046 will include the name of the field (i.e. "name") in the message and mozilla/addons#804 does something similar for [2] so I'm going to downgrade this.

caitmuenster commented 5 years ago

This is a good issue for folks who have contributed to the add-ons server before. @eviljeff can provide guidance.

dsychin commented 5 years ago

Hi, can I give this issue a try?

caitmuenster commented 5 years ago

Go for it, @dsychin!

dsychin commented 5 years ago

I am running into a problem trying to replicate the error. Every time I fill in the fields and press Finish Theme, it just gets stuck at "Uploading Theme".

This is what I get in my terminal.

Click to see logs

``` addons-frontend_1 addons-frontend_1 | [1] addons-frontend_1 addons-frontend_1 | [1] addons-frontend_1 addons-frontend_1 | [1] addons-frontend_1 addons-frontend_1 | [1] addons-frontend_1 nginx_1 addons-frontend_1 addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] addons-frontend_1 | [1] nginx_1 autograph_1 ``` | [1] [1557569375801850000] WARN (amo/146 on 2e484ae7336d): CSP has been disabled from the config amo_request_id: "01754654-3120-4937-b095-9af43fff4f9a" | [1] [1557569375802117600] INFO (amo/146 on 2e484ae7336d): Replacing lang in URL en-US -> en-US amo_request_id: "01754654-3120-4937-b095-9af43fff4f9a" | [1] [1557569375802168300] INFO (amo/146 on 2e484ae7336d): Exception in URL found; we fallback to addons-server. amo_request_id: "01754654-3120-4937-b095-9af43fff4f9a" | [1] [1557569375802466800] WARN (amo/146 on 2e484ae7336d): Ignoring error for /en-US/developers/upload because a response was already sent; error: Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client amo_request_id: "01754654-3120-4937-b095-9af43fff4f9a" | [2] [1557569375803] DEBUG (amo.proxy/87 on 2e484ae7336d): 404 ~> http://0.0.0.0:3333/en-US/developers/upload | 2019/05/11 10:09:35 [warn] 7#7: *65 a client request body is buffered to a temporary file /var/cache/nginx/client_temp/0000000003, client: 172.18.0.1, server: , request: "POST /en-US/developers/upload HTTP/1.1", host: "olympia.test", referrer: "http://olympia.test/en-US/developers/addon/submit/wizard-listed" | [1] Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client at ServerResponse.setHeader (_http_outgoing.js:470:11) at ServerResponse.header (/srv/node/node_modules/express/lib/response.js:767:10) at ServerResponse.location (/srv/node/node_modules/express/lib/response.js:884:15) at ServerResponse.redirect (/srv/node/node_modules/express/lib/response.js:922:18) at redirect (/srv/code/src/core/middleware/trailingSlash.js:38:16) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at next (/srv/code/src/core/middleware/prefixMiddleware.js:104:14) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at /srv/node/node_modules/universal-cookie-express/cjs/index.js:34:5 at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at serveStatic (/srv/node/node_modules/serve-static/index.js:75:16) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at next (/srv/code/src/core/middleware/security.js:21:5) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at xXssProtection (/srv/node/node_modules/x-xss-protection/index.js:26:7) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at nosniff (/srv/node/node_modules/dont-sniff-mimetype/index.js:4:5) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at frameguard (/srv/node/node_modules/frameguard/dist/index.js:55:9) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at hsts (/srv/node/node_modules/hsts/index.js:52:5) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at next (/srv/code/src/core/middleware/requestId.js:21:3) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at ns.run (/srv/node/node_modules/express-http-context/index.js:10:15) at Namespace.run (/srv/node/node_modules/cls-hooked/context.js:97:5) at middleware (/srv/node/node_modules/express-http-context/index.js:10:5) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at expressInit (/srv/node/node_modules/express/lib/middleware/init.js:40:5) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at query (/srv/node/node_modules/express/lib/middleware/query.js:45:5) at Layer.handle [as handle_request] (/srv/node/node_modules/express/lib/router/layer.js:95:5) at trim_prefix (/srv/node/node_modules/express/lib/router/index.js:317:13) at /srv/node/node_modules/express/lib/router/index.js:284:7 at Function.process_params (/srv/node/node_modules/express/lib/router/index.js:335:12) at next (/srv/node/node_modules/express/lib/router/index.js:275:10) at Function.handle (/srv/node/node_modules/express/lib/router/index.js:174:3) at Function.handle (/srv/node/node_modules/express/lib/application.js:174:10) at Server.app (/srv/node/node_modules/express/lib/express.js:39:9) at Server.emit (events.js:189:13) at Server.EventEmitter.emit (domain.js:441:20) at parserOnIncoming (_http_server.js:676:12) at HTTPParser.parserOnHeadersComplete (_http_common.js:109:17) | 172.18.0.1 - - [11/May/2019:10:09:35 +0000] "POST /en-US/developers/upload HTTP/1.1" 500 54007 "http://olympia.test/en-US/developers/addon/submit/wizard-listed" "Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:66.0) Gecko/20100101 Firefox/66.0" "-" | {"Timestamp":1557569399796330975,"Time":"2019-05-11T10:09:59Z","Type":"app.log","Logger":"autograph","Hostname":"09250684ac6c","EnvVersion":"2.0","Pid":6,"Severity":4,"Fields":{"msg":"Error sending gauge xpi.rsa_cache.chan_cap: write udp 127.0.0.1:54343-\u003e127.0.0.1:8125: write: connection refused"}}

eviljeff commented 5 years ago

@EnTeQuAk have you seen that autograph error before? ^

EnTeQuAk commented 5 years ago

I haven't but I'll ask around. Does restarting autograph docker-compose stop autograph && docker-compose up -d autograph work maybe?

An update of autograph is upcoming anyway, so maybe that fixes any potential issues too. I'll update this issue accordingly in a bit.

dsychin commented 5 years ago

@EnTeQuAk It's still the same problem after restarting autograph. I have tried pulling the latest version of this repository and re-running docker-compose pull and make initialize_docker but I still could not get it to work.

avgupt commented 4 years ago

@eviljeff are the errors supposed to just go to the top or we need to find the input causing the error and then place it accordingly? If such errors can only be generated by Theme names then we can use the easier way.

AlexandraMoga commented 4 years ago

@avgupt errors can also be generated by the header upload component (see screenshot) so I think the best approach would be to connect the error messages to the actual field that generated them. For example, we would not like to see this upload limit error next to the theme name field (assuming that the easier implementation is chosen).

AlexandraMoga commented 4 years ago

The errors mentioned above are displayed next to the triggering field now:

mozilla / addons

Error messages in the static theme submission wizard should be displayed next to the field that generated them #6047