Add optional "comment" field for emergency_shutoff #2604
Conversation
| <Button | ||
| disabled={actionExecuting} | ||
| onClick={onClose} | ||
| action={actions => actions && actions.focusVisible()}> |
There was a problem hiding this comment.
This behavior (fucusVisible()) makes typing the comment impossible. Seems not a big problem remove it. Let me know if has any other approach.
There was a problem hiding this comment.
@gabrielBusta may have input on this, I'm afraid this is beyond me :/
There was a problem hiding this comment.
I am not worried about it, but there might be a reason the code is calling focusVisible there. So I will take a closer look
There was a problem hiding this comment.
@jcristau this code was focusing / highlighting the cancel button on the dialogs (i.e. the signing-off pop-ups) This was probably done so pressing enter would cancel the action / close the pop-up.
I think this change is an improvement. Now we can tab through the buttons in the dialogs and press enter to "click" them (this is the way they should behave imo). Also the button wont take away the focus from other UI elements we might want to put in the body of these dialogs (e.g. @allan-silva's TextField)
There was a problem hiding this comment.
@gabrielBusta Thanks for take a close look on this!
Add comment field on spec; Insert comment on database Add comment information on balrog ui
|
@jcristau My last contribution was long time ago, let me know if I missed any step. |
allan-silva
requested review from
jcristau,
gbrownmozilla and
gabrielBusta
as code owners
There was a problem hiding this comment.
This looks good to me. Thank you very much!
Since @jcristau originally opened the issue, I'd like him to give final approval here; he is on vacation currently - hope you don't mind waiting a couple of weeks.
|
@gbrownmozilla no problem. |
src/auslib/db.py
Outdated
| @@ -2912,6 +2912,7 @@ def __init__(self, db, metadata, dialect): | |||
| metadata, | |||
| Column("product", String(15), nullable=False, primary_key=True), | |||
| Column("channel", String(75), nullable=False, primary_key=True), | |||
| Column("comment", String(500), nullable=True), | |||
There was a problem hiding this comment.
Let's omit nullable=True, it's the default and we don't seem to have it anywhere else
| <Button | ||
| disabled={actionExecuting} | ||
| onClick={onClose} | ||
| action={actions => actions && actions.focusVisible()}> |
There was a problem hiding this comment.
@gabrielBusta may have input on this, I'm afraid this is beyond me :/
|
|
||
| def test_get_notfound(self): | ||
| resp = self._get("/emergency_shutoff/foo/bar") | ||
| self.assertStatusCode(resp, 404) | ||
|
|
||
| def test_create(self): | ||
| data = {"product": "Thunderbird", "channel": "release"} | ||
| data = {"product": "Thunderbird", "channel": "release", "comment": "Thunderbird panic!()"} |
There was a problem hiding this comment.
We should keep a test for POST to /emergency_shutoff without a comment, to make sure that still works.
There was a problem hiding this comment.
Looks good to me. Deferring to @jcristau for final approval here. Thank you!
Changes on balrogui:
Resolves #1862