Closed Bug 1198978 Opened 9 years ago Closed 9 years ago

Pressing "Save" when no jobs are selected on the pinboard clears the classification/comment

Categories

(Tree Management :: Treeherder, defect, P3)

Product:
Tree Management
Component:
Treeherder
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dholbert, Assigned: crosscent)

Details

Attachments

(3 files, 1 obsolete file)

screencast of bug
1.05 MB, video/ogg
Details
PR
47 bytes, text/x-github-pull-request
wlach
: review+
Details | Review
[treeherder] wlach:1198978-followup > mozilla:master
47 bytes, text/x-github-pull-request
camd
: review+
Details | Review
STR: 1. Visit a TreeHerder page, e.g. https://treeherder.mozilla.org/#/jobs?repo=mozilla-central 2. Click some job. 3. Click "Open Pinboard". 4. (note that your job is not actually selected in the pinboard yet) 5. Type in a comment and click "Save" EXPECTED RESULTS: Nothing should happen (or perhaps an error message should appear), because "Save" is disabled, because no jobs are selected. ACTUAL RESULTS: The comment I typed in gets cleared!!
Attached video screencast of bug —
Sorry, my STR were missing a step: (0) Be logged into TreeHerder (so that you can post comments). (If you aren't logged in and perform my STR, you *do* get a helpful message telling you that you need to log in before you can submit comments -- and the comment field isn't cleared, which is nice. But after you log in and repeat the STR, it does get cleared.) Here's a screencast demonstrating the bug. I'm using current Firefox nightly on 64-bit linux, if it matters.
I can take this and hopefully have time to work on it Friday morning. If I don't get to it then, I won't get to it until at least Tuesday. Feel free to unassign me if someone can get to this earlier than that. :)
Assignee: nobody → wkocher
Thank you for filing - agree this is suboptimal UX.
Priority: -- → P3
Summary: My carefully-entered classification / comment gets clobbered if I accidentally click "Save" when no builds are selected → Pressing "Save" when no jobs are selected on the pinboard clears the classification/comment
Attached file PR 925 (obsolete) —
I think this should do it. Unsure if I need to also add this check to saveClassificationOnly() and saveBugsOnly().
Attachment #8654151 - Flags: review?(emorley)
Comment on attachment 8654151 [details] [review] PR 925 Redirecting to Cameron, since UK public holiday today.
Attachment #8654151 - Flags: review?(emorley) → review?(cdawson)
Comment on attachment 8654151 [details] [review] PR 925 And back to Ed. :)
Attachment #8654151 - Flags: review?(cdawson) → review?(emorley)
Comment on attachment 8654151 [details] [review] PR 925 Taking this since Cameron is on PTO and Wes asked about it on IRC. I think the better approach here would be to disable the save button instead (it makes more sense to see it disabled, rather than press it and not have anything happen and wonder why). ...but Cameron may have a better suggestion for this (I'm not that familiar with the pinboard).
Attachment #8654151 - Flags: review?(emorley) → review-
Hrm, the Save button is ng-disabled when there's no pinned jobs [1], but the button itself is a <span> element, not an <input> [2], so I guess disabling it doesn't prevent the click event from being fired? 1. https://github.com/mozilla/treeherder/blob/62f23396680f14cf075dedea2cb427531eb95038/ui/partials/main/thPinboardPanel.html#L73 2. https://github.com/mozilla/treeherder/blob/62f23396680f14cf075dedea2cb427531eb95038/ui/partials/main/thPinboardPanel.html#L70
Flags: needinfo?(emorley)
Yeah I see what you mean. There seem to be some suggestions here: https://www.google.co.uk/search?q=angular+ng-disabled+click+event Also the styling of the save button (specifically the text colour / button colour) could be made a little more obviously disabled.
Flags: needinfo?(emorley)
Comment on attachment 8654151 [details] [review] PR 925 Okay, that seems to work locally, and it didn't break Travis, so here we go.
Attachment #8654151 - Flags: review- → review?(cdawson)
Comment on attachment 8654151 [details] [review] PR 925 I hate to give you another r-. Nothing personal. I like you as a human being, still! :) Also, that was a clever trick that I'll place in the back of my head to use sometime. :)
Attachment #8654151 - Flags: review?(cdawson) → review-
This bit me again today; I'd love to see this fixed!
Do you have time to finish this Wes? I can take a look if you're too swamped.
Flags: needinfo?(wkocher)
Please do!
Assignee: wkocher → nobody
Flags: needinfo?(wkocher)
Ok, I'll take this. :)
Assignee: nobody → wlachance
@Wlach. I am new to this whole thing, but is it possible for me to take over this? I think I have a solution for this.
(In reply to crosscent from comment #16) > @Wlach. I am new to this whole thing, but is it possible for me to take over > this? I think I have a solution for this. Yes, if you want to do this feel free -- I haven't started yet. You can use :KWierso's pull request (attached) as a basis for your work, just bear in mind the suggested changes that :camd made.
Assignee: wlachance → crosscent
(In reply to William Lachance (:wlach) from comment #17) > (In reply to crosscent from comment #16) > > @Wlach. I am new to this whole thing, but is it possible for me to take over > > this? I think I have a solution for this. > > Yes, if you want to do this feel free -- I haven't started yet. You can use > :KWierso's pull request (attached) as a basis for your work, just bear in > mind the suggested changes that :camd made. Also, you've probably already seen this, but you can find out more than probably ever wanted to know about treeherder here: https://wiki.mozilla.org/EngineeringProductivity/Projects/Treeherder In particular if you have questions feel free to pop onto IRC (#treeherder channel on irc.mozilla.org)
(In reply to William Lachance (:wlach) from comment #18) > (In reply to William Lachance (:wlach) from comment #17) > > (In reply to crosscent from comment #16) > > > @Wlach. I am new to this whole thing, but is it possible for me to take over > > > this? I think I have a solution for this. > > > > Yes, if you want to do this feel free -- I haven't started yet. You can use > > :KWierso's pull request (attached) as a basis for your work, just bear in > > mind the suggested changes that :camd made. > > Also, you've probably already seen this, but you can find out more than > probably ever wanted to know about treeherder here: > > https://wiki.mozilla.org/EngineeringProductivity/Projects/Treeherder > > In particular if you have questions feel free to pop onto IRC (#treeherder > channel on irc.mozilla.org) Thanks wlach! I am not too sure what the workflow should be, but I have sent a PR already. If I missed a step, please let me know!
Attached file PR —
Attachment #8654151 - Attachment is obsolete: true
(In reply to crosscent from comment #19) > Thanks wlach! I am not too sure what the workflow should be, but I have sent > a PR already. If I missed a step, please let me know! Creating a new PR is the right idea, but it needs to be formatted a certain way and attached to the bug (normally autolander would do this automatically, but in your case it didn't because it wasn't formatted properly, so I did this for you). When you've addressed my comments in the PR, please set the review flag (also described in the PR) and we can get this resolved!
(In reply to William Lachance (:wlach) from comment #21) > Creating a new PR is the right idea, but it needs to be formatted a certain > way and attached to the bug (normally autolander would do this > automatically, but in your case it didn't because it wasn't formatted > properly, so I did this for you). When you've addressed my comments in the > PR, please set the review flag (also described in the PR) and we can get > this resolved! I have created a new commit! I am assuming that the review flag is the eyes at the bottom, and I have clicked it to be green. I assume that this will be reviewed by someone else later, but I am not too sure, so I have not pressed the publish button yet.
(In reply to crosscent from comment #22) > (In reply to William Lachance (:wlach) from comment #21) > I have created a new commit! I am assuming that the review flag is the eyes > at the bottom, and I have clicked it to be green. I assume that this will be > reviewed by someone else later, but I am not too sure, so I have not pressed > the publish button yet. You need to set the review flag here in bugzilla. :) I've already attached the PR to this bug for you, so all you need to do is click on "details" on the PR attachment above, then set the review combo box to r? and select me (:wlach) as the reviewer. There are a few changes I requested in the PR. Also, you should squash all the intermediary commits into one via a git rebase. Once that's done, select review according to the instructions above. I know this seems like a lot of process at first but I promise it will eventually seem like second nature.
(In reply to William Lachance (:wlach) from comment #23) > You need to set the review flag here in bugzilla. :) I've already attached > the PR to this bug for you, so all you need to do is click on "details" on > the PR attachment above, then set the review combo box to r? and select me > (:wlach) as the reviewer. > > There are a few changes I requested in the PR. Also, you should squash all > the intermediary commits into one via a git rebase. Once that's done, select > review according to the instructions above. I know this seems like a lot of > process at first but I promise it will eventually seem like second nature. I have rebased and squash all intermediary commits into one. I had to force push it due to rebasing, and it seems that I have to create a new PR? I'm sorry for taking such a long time to to make a proper PR for such a small change. I really hope I am doing this correct, I am actually really happy that you are willing to help me through this process!
As long as you pushed to the same branch, the pull request should just accept it. It'll invalidate any comments left to the original commits, but that should be fine.
Attachment #8734386 - Flags: review?(wlachance)
Keywords: autoland
Comment on attachment 8734386 [details] [review] PR Looks great now, thanks!
Attachment #8734386 - Flags: review?(wlachance) → review+
Keywords: autoland
Commit pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/590d688eadebaa0a6c5726fcbffdc6634c9b140d Bug 1198978 - Disable save button if no job * Changed span to button * added tooltip for disabled button
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This broke keyboard input after classification on Firefox on Linux/Windows, for some reason which I don't understand. I think this is a platform bug, but I don't have time to do a minimal test case right now, so let's just hack around it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8735997 [details] [review] [treeherder] wlach:1198978-followup > mozilla:master Hey Cam, I wish I had a better solution for this but I don't know of one. :(
Attachment #8735997 - Flags: review?(cdawson)
Comment on attachment 8735997 [details] [review] [treeherder] wlach:1198978-followup > mozilla:master wfm. :)
Attachment #8735997 - Flags: review?(cdawson) → review+
Commits pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/e04077e64ffd1041f653696874d8e77b08e9f8fe Bug 1198978 - Fix keyboard input after job classification For some reason changing the span to a button broke keyboard input after job classification on Firefox on Linux/Windows (not Mac, any flavor of Chrome also works fine). Blurring the input element seems to fix this. https://github.com/mozilla/treeherder/commit/566c0318adf41ee65a4bac4e0e8b1af8b329a48e Merge pull request #1376 from wlach/1198978-followup Bug 1198978 - Fix keyboard input after job classification
Camd pushed the fix to stage and it appears to work correctly for me on Firefox on Windows and Linux. Holding off on pushing to prod at just before 5pm because that's almost never the right thing to do. If anyone's stuck trying to sheriff tonight on Firefox on Windows or Linux and is getting caught by this, running the following snippet in a web console tied to your current treeherder tab will temporarily fix it until you reload the page: document.getElementById("pinboard-controls").getElementsByTagName("button")[0].addEventListener("click", function() {document.activeElement.blur()}, false) You'll have to do it separately for each treeherder tab, but it unbreaks my muscle memory, which is always a good thing. :)
This got pushed to production this morning and seems to be working.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: