diff --git a/README.md b/README.md index 2861278..8256fa0 100644 --- a/README.md +++ b/README.md @@ -4,6 +4,23 @@ Bugherder is a tool for marking bugs post-merge, created by [Graeme McCutcheon](http://www.graememcc.co.uk/). +RESTRICTED BUGS +--------------- +Bugherder loads bug data anonymously, so bugs that are restricted to a Bugzilla group can't +be loaded, and can't be marked. Sheriffs generally don't have access to them either, so they +are normally left for someone from Release Management to deal with by hand afterwards. + +When a push contains such bugs, bugherder offers to load them with an API key. Supply one +belonging to an account that can see them, and bugherder will restart the normal flow with +everything except those bugs filtered out - the same commenting, resolving and flag setting as +usual, over just the restricted bugs. The key is reused for the submission, so it is only asked +for once. Anything that still can't be loaded with the key given is called out so it can be +followed up manually. + +This doesn't affect the sheriff pass in any way: without a key, nothing loads and the push is +presented exactly as before. + + NOTES ON TESTING ---------------- Adding "?debug=1" shows how all changesets were identified, and shows what changesets Bugherder decided were affected by a backout. diff --git a/bugherder/css/style.css b/bugherder/css/style.css index 30c430b..d5d21d1 100644 --- a/bugherder/css/style.css +++ b/bugherder/css/style.css @@ -335,6 +335,24 @@ div.ctr { color: rgb(51,51,51); } +#restricted { + max-width: 940px; + margin: 10px auto 0; + padding: 0 10px; +} + +#restrictedText { + display: inline-block; + border: 1px solid rgb(255,213,153); + background-color: rgb(255,239,217); + color: rgb(51,51,51); + padding: 2px 5px; +} + +#restrictedButton { + margin-left: 10px; +} + #loadingOverlay { padding-left: 25px; } diff --git a/bugherder/index.html b/bugherder/index.html index 6765b18..85efaf5 100644 --- a/bugherder/index.html +++ b/bugherder/index.html @@ -45,6 +45,11 @@

A tool to help with post-merge Bugzilla administrivia. +
+ + +
+
diff --git a/bugherder/js/BugData.js b/bugherder/js/BugData.js index 26eb01a..9e163e2 100644 --- a/bugherder/js/BugData.js +++ b/bugherder/js/BugData.js @@ -4,11 +4,17 @@ var BugData = { bugs: {}, trackingFlag: null, statusFlag: null, - fields: 'id,resolution,status,whiteboard,keywords,target_milestone,summary,product,component,flags,assigned_to', + fields: 'id,resolution,status,whiteboard,keywords,target_milestone,summary,product,component,flags,assigned_to,groups', notYetLoaded: [], loadCallback: null, errorCallback: null, checkComments: false, + apiKey: null, + + setApiKey: function BD_setApiKey(key) { + this.apiKey = key || null; + }, + load: function BD_load(bugs, checkComments, loadCallback, errorCallback) { this.notYetLoaded = bugs; @@ -56,7 +62,7 @@ var BugData = { self.parseData(data); }; - var bugzilla = bz.createClient({timeout: timeout}); + var bugzilla = bz.createClient({timeout: timeout, api_key: this.apiKey}); bugzilla.searchBugs(bugs, callback); }, @@ -109,9 +115,16 @@ var BugData = { bug.isUnassigned = /^nobody@(?:mozilla.org|nss.bugs)$/.test(bugObj.assigned_to.name); + bug.securityGroups = (bugObj.groups || []).filter(function BD_isSecurityGroup(group) { + return Config.securityGroupRE.test(group); + }); + bug.canSecurityRelease = bug.securityGroups.length > 0; + bug.intestsuite = ' '; bug.testsuiteFlagID = -1; - bug.canSetTestsuite = ConfigurationData.hasTestsuiteFlag[bug.product][bugObj.component]; + // The configuration is loaded anonymously, so it omits logged-in-only products + var componentFlags = ConfigurationData.hasTestsuiteFlag[bug.product]; + bug.canSetTestsuite = !!(componentFlags && componentFlags[bugObj.component]); if (bug.canSetTestsuite && 'flags' in bugObj && bugObj.flags) { for (var i = 0; i < bugObj.flags.length; i++) { var f = bugObj.flags[i]; diff --git a/bugherder/js/Config.js b/bugherder/js/Config.js index 9059f40..86ba649 100644 --- a/bugherder/js/Config.js +++ b/bugherder/js/Config.js @@ -11,6 +11,11 @@ var Config = { hgPushlogURL: "https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=", showBugURL: "https://bugzilla.mozilla.org/show_bug.cgi?id=", + // Matched by shape, not by name: Bugzilla only advertises the core-security groups + // that still accept new bugs, so any fixed list misses most of the ones in use + securityGroupRE: /^(?:[a-z0-9-]+-)?core-security$/, + securityReleaseGroup: "core-security-release", + // Here be dragons versionRE: /^mozilla\d+$/i, csetInputRE: /^(tip|[\da-f]{12,40})$/i, diff --git a/bugherder/js/Step.js b/bugherder/js/Step.js index a721523..332072a 100644 --- a/bugherder/js/Step.js +++ b/bugherder/js/Step.js @@ -5,10 +5,25 @@ // bugs should be commented with the url for a particular push, along with the comment to // be written for each push/bug combination. A step is also responsible for transmitting the // relevant changes to Bugzilla -function Step(name, callbacks, isBackout) { +// bugFilter, if given, holds the only bug numbers this step should concern itself +// with; changesets carrying none of them are left out entirely +function Step(name, callbacks, isBackout, bugFilter) { var self = this; + function isWanted(bugID) { + return !self.bugFilter || (bugID in self.bugFilter); + } + + function hasWantedBug(index) { + var push = PushData.allPushes[index]; + if (push.bug) + return isWanted(push.bug); + if (push.backoutBugs) + return push.backoutBugs.some(isWanted); + return false; + } + function constructAttachedBugs(useBackouts) { var arr = PushData[self.name]; if (useBackouts) @@ -16,22 +31,65 @@ function Step(name, callbacks, isBackout) { var len = arr.length; for (var i = 0; i < len; i++) { + // Anything attached to a changeset we don't draw would submit unseen + if (self.bugFilter && !(arr[i] in self.rendered)) + continue; + var push = PushData.allPushes[arr[i]]; if (push.bug) { + if (!isWanted(push.bug)) + continue; self.attachedBugs[arr[i]] = {}; self.attachBugToCset(arr[i], push.bug); } else if (push.backoutBugs && push.backoutBugs.length > 0) { + var backoutBugs = push.backoutBugs.filter(isWanted); + if (backoutBugs.length == 0) + continue; self.attachedBugs[arr[i]] = {}; - var l2 = push.backoutBugs.length; + var l2 = backoutBugs.length; for (var j = 0; j < l2; j++) - self.attachBugToCset(arr[i], push.backoutBugs[j]); + self.attachBugToCset(arr[i], backoutBugs[j]); } } } + // Unfiltered, that's every changeset in the category, including those without a bug + // so that they can be given one + function constructPushList() { + if (!self.bugFilter) { + self.pushes = PushData[self.name]; + return; + } + + if (!self.hasBackouts) { + self.pushes = PushData[self.name].filter(hasWantedBug); + self.pushes.forEach(function Step_markRendered(index) { + self.rendered[index] = true; + }); + return; + } + + PushData[self.name].forEach(function Step_filterBackout(index) { + var affected = PushData.allPushes[index].affected.filter(hasWantedBug); + if (affected.length == 0 && !hasWantedBug(index)) + return; + + self.pushes.push(index); + self.affected[index] = affected; + self.rendered[index] = true; + affected.forEach(function Step_markAffectedRendered(j) { + self.rendered[j] = true; + }); + }); + } + this.name = name; this.callbacks = callbacks; this.hasBackouts = isBackout; + this.bugFilter = bugFilter || null; + this.pushes = []; + this.affected = {}; + this.rendered = {}; this.attachedBugs = {}; this.bugInfo = {}; @@ -53,6 +111,7 @@ function Step(name, callbacks, isBackout) { } this.unprivilegedLoader = bz.createClient(options); + constructPushList(); constructAttachedBugs(false); if (this.hasBackouts) constructAttachedBugs(true); @@ -64,11 +123,26 @@ Step.prototype.getName = function Step_getName() { }; +Step.prototype.getPushes = function Step_getPushes() { + return this.pushes; +}; + + +Step.prototype.getAffected = function Step_getAffected(index) { + if (index in this.affected) + return this.affected[index]; + + return PushData.allPushes[index].affected; +}; + + Step.prototype.canSubmit = function Step_canSubmit() { for (var bug in this.bugInfo) { var info = this.bugInfo[bug]; if ((info.canResolve && info.shouldResolve)) return true; + if (this.canSecurityRelease(bug) && info.shouldSecurityRelease) + return true; } for (var cset in this.attachedBugs) { @@ -87,11 +161,6 @@ Step.prototype.getSentData = function Step_getSentData() { }; -Step.prototype.hasSecurityBugs = function Step_hasSecurityBugs() { - return this.securityBugs.length > 0; -}; - - Step.prototype.getSecurityBugs = function Step_getSecurityBugs() { var secBugs = []; for (var cset in this.attachedBugs) { @@ -144,6 +213,13 @@ Step.prototype.createBug = function Step_createBug(bugID, info) { changed = true; } + // A change in its own right, so unticking the resolution can't drop it in silence + if (this.canSecurityRelease(bugID) && info.shouldSecurityRelease) { + bug.groups = {add: [Config.securityReleaseGroup], + remove: BugData.bugs[bugID].securityGroups}; + changed = true; + } + if (changed) { bug.id = bugID; @@ -393,6 +469,13 @@ Step.prototype.postSubmit = function Step_postSubmit(i) { } + if ('groups' in sent) { + info.canSecurityRelease = false; + info.shouldSecurityRelease = false; + BugData.bugs[bugID].securityGroups = []; + BugData.bugs[bugID].canSecurityRelease = false; + } + // Update the intestsuite flag if we sent it if ('flags' in sent) { info.intestsuite = sent.flags[0].status; @@ -508,12 +591,12 @@ Step.prototype.attachBugToCset = function Step_attachBugToCset(index, bugID) { if (bug) { leaveOpen = bug.leaveOpen; hasMilestone = bug.milestone != '---'; - if (hasMilestone || leaveOpen || !(Config.treeName == 'mozilla-central' || Config.treeName == 'comm-central')) + var productMilestones = ConfigurationData.milestones[bug.product]; + if (hasMilestone || leaveOpen || !productMilestones || + !(Config.treeName == 'mozilla-central' || Config.treeName == 'comm-central')) milestone = bug.milestone; - else { - var defaultMilestone = ConfigurationData.milestones[bug.product].defaultIndex; - milestone = ConfigurationData.milestones[bug.product].values[defaultMilestone]; - } + else + milestone = productMilestones.values[productMilestones.defaultIndex]; } if (!(bugID in this.bugInfo)) { @@ -525,6 +608,8 @@ Step.prototype.attachBugToCset = function Step_attachBugToCset(index, bugID) { shouldReopen: false, canSetStatus: false, shouldSetStatus: false, + canSecurityRelease: false, + shouldSecurityRelease: false, canSetTestsuite: bug && bug.canSetTestsuite, milestone: milestone, tags: PushData.allPushes[index].tags}; @@ -551,6 +636,17 @@ Step.prototype.attachBugToCset = function Step_attachBugToCset(index, bugID) { this.bugInfo[bugID].canSetStatus = true; } + // Applies on the trees where landing means fixed, including to a bug an earlier + // pass already resolved + if (bug && bug.canSecurityRelease && (isMC || Config.treeName == 'comm-central')) { + this.bugInfo[bugID].canSecurityRelease = true; + + // Only default it on where this push leaves the fix in the tree + var landed = !PushData.allPushes[index].backedOut && !this.bugInfo[bugID].shouldReopen; + this.bugInfo[bugID].shouldSecurityRelease = this.bugInfo[bugID].shouldResolve || + (landed && bug.resolution == 'FIXED'); + } + // Allow setting of intestsuite if possible if (bug && bug.canSetTestsuite) this.bugInfo[bugID].intestsuite = bug.intestsuite; @@ -647,6 +743,9 @@ Step.prototype.updateShouldSetStatusAfterResolve = function Step_updateShouldSet arr.splice(i, 1); } + if (this.bugInfo[bugID].canSecurityRelease) + this.bugInfo[bugID].shouldSecurityRelease = should; + if (!this.bugInfo[bugID].canSetStatus) return; @@ -718,6 +817,32 @@ Step.prototype.canReopen = function Step_canReopen(bugID) { }; +Step.prototype.shouldSecurityRelease = function Step_shouldSecurityRelease(bugID) { + if (!(bugID in this.bugInfo)) + return false; + + return this.bugInfo[bugID].shouldSecurityRelease; +}; + + +Step.prototype.canSecurityRelease = function Step_canSecurityRelease(bugID) { + if (!(bugID in this.bugInfo)) + return false; + + // A bug can appear in more than one step, so it may already have been moved + return this.bugInfo[bugID].canSecurityRelease && BugData.bugs[bugID] && + BugData.bugs[bugID].securityGroups.length > 0; +}; + + +Step.prototype.setShouldSecurityRelease = function Step_setShouldSecurityRelease(bugID, should) { + if (!(bugID in this.bugInfo)) + return; + + this.bugInfo[bugID].shouldSecurityRelease = should; +}; + + Step.prototype.setShouldReopen = function Step_setShouldReopen(bugID, should) { if (!(bugID in this.bugInfo)) return; @@ -859,6 +984,10 @@ Step.prototype.getProp = function Step_getProp(index, bugID, prop) { return this.shouldReopen(bugID); if (prop == 'canReopen') return this.canReopen(bugID); + if (prop == 'shouldSecurityRelease') + return this.shouldSecurityRelease(bugID); + if (prop == 'canSecurityRelease') + return this.canSecurityRelease(bugID); return false; }; @@ -939,6 +1068,7 @@ Step.prototype.getAdditionalHelpText = function Step_getAdditionalHelpText() { var milestonePost = ' a milestone set. You may wish to check it is correct before submitting.'; var alreadyCommentPost = ' to have already been commented with the correct changeset URL, so commenting there has been disabled.'; var statusChangePost = ' tracked or uplifted and will have ' + bugherder.statusFlag + ' set to "fixed".'; + var securityReleasePost = ' restricted, and will be moved to ' + Config.securityReleaseGroup + '.'; var hashave = {singular: 'has', plural: 'have'}; var appearTo = {singular: 'appears', plural: 'appear'}; @@ -967,6 +1097,14 @@ Step.prototype.getAdditionalHelpText = function Step_getAdditionalHelpText() { if (this.statusChangeBugs.length > 0) text += this.constructTextFor(this.statusChangeBugs, statusChangePost, isare); + var securityRelease = []; + for (var bugID in this.bugInfo) + if (this.canSecurityRelease(bugID) && this.bugInfo[bugID].shouldSecurityRelease) + securityRelease.push(bugID); + + if (securityRelease.length > 0) + text += this.constructTextFor(securityRelease, securityReleasePost, isare, true); + return text; }; @@ -983,7 +1121,7 @@ Step.prototype.setMaxStepNumber = function Step_setMaxStepNumber(num) { // Return the user-visible step name to be shown for this step Step.prototype.getHeading = function Step_getHeading(addMax) { - addMax = addMax || true; + addMax = addMax !== false; var res = this.name; if (this.name in Step.headings) diff --git a/bugherder/js/Summary.js b/bugherder/js/Summary.js index 4ac7111..b759928 100644 --- a/bugherder/js/Summary.js +++ b/bugherder/js/Summary.js @@ -23,6 +23,11 @@ var Summary = { else html += ' '; html += ''; + if ('groups' in data) + html += UI.htmlEncode(data.groups.add.join(', ')); + else + html += ' '; + html += ''; if ('comment' in data) { var comment = data.comment.body; comment = comment.replace(/\n/g, '
'); @@ -45,7 +50,8 @@ var Summary = { } html += '
'; - html += ''; + html += ''; + html += ''; html += sent.map(function(data) {return this.makeSummaryForData(data);}, this).join(''); html += '
BugResolved?Reopened?Target MilestoneAssigneeComment
Reopened?Target MilestoneAssigneeMoved to groupComment
'; return html; @@ -70,17 +76,31 @@ var Summary = { makeSecBugHTML: function summary_makeSecBugHTML(steps) { var sechtml = ''; + + // Steps set aside during this session cover the same changesets as their + // replacements, so the same bug can be reported by more than one of them + var seen = {}; + function unseen(secBug) { + var key = secBug.cset + ':' + secBug.bug; + if (key in seen) + return false; + + seen[key] = true; + return true; + } + for (var i = 0; i < steps.length; i++) { - if (steps[i].hasSecurityBugs()) { - sechtml += '
  • '+steps[i].getHeading(false) + '
    '; - var sb = steps[i].getSecurityBugs(); - sechtml += ''; - for (var j = 0; j < sb.length; j++) { - sechtml += ''; - } - sechtml += '
    ChangesetLinkBug
    ' + sb[j].cset + '' + UI.linkifyRevURL(sb[j].link); - sechtml += '' + UI.linkifyBug(sb[j].bug) + '
    '; - } + var sb = steps[i].getSecurityBugs().filter(unseen); + if (sb.length == 0) + continue; + + sechtml += '
  • '+steps[i].getHeading(false) + '
    '; + sechtml += ''; + for (var j = 0; j < sb.length; j++) { + sechtml += ''; + } + sechtml += '
    ChangesetLinkBug
    ' + sb[j].cset + '' + UI.linkifyRevURL(sb[j].link); + sechtml += '' + UI.linkifyBug(sb[j].bug) + '
    '; } if (sechtml == '') @@ -104,24 +124,34 @@ var Summary = { }, - view: function summary_View(steps, onPrevious, onNext) { + view: function summary_View(steps, onPrevious, onNext, priorSteps) { + priorSteps = priorSteps || []; + + // Only replaced steps that submitted something belong in the activity list and the + // unsubmitted warning. All of them still know which bugs could not be loaded, so the + // security bug table gets the lot + var submitted = priorSteps.filter(function summary_hasSubmitted(step) { + return step.getSentData().length > 0; + }); + var activeSteps = submitted.concat(steps); + // Hide any previous viewer output UI.hide('viewerOutput'); UI.clearErrorMessage(); $('#viewerOutput').empty(); $('#viewerOutput').append(this.makeButtonHTML(onPrevious.label, onNext.label)); - var subHTML = this.makeUnsubmittedHTML(steps); + var subHTML = this.makeUnsubmittedHTML(activeSteps); if (subHTML != '') $('#viewerOutput').append(subHTML + '
    '); - var secHTML = this.makeSecBugHTML(steps); + var secHTML = this.makeSecBugHTML(priorSteps.concat(steps)); if (secHTML != '') $('#viewerOutput').append(secHTML + '
    '); $('#viewerOutput').append('

    Summary of activity

    '); - steps.forEach(function step_viewSummaryMaker(step){ + activeSteps.forEach(function step_viewSummaryMaker(step){ $('#viewerOutput').append(this.makeSummaryForStep(step)); }, this); diff --git a/bugherder/js/UI.js b/bugherder/js/UI.js index f4c30c0..b2cf502 100644 --- a/bugherder/js/UI.js +++ b/bugherder/js/UI.js @@ -169,6 +169,10 @@ var UI = { showModalForm: function UI_showModalForm(id, formID, submitAction, cancelID, cancelAction) { + // The toggles below would put an already-open form straight back down again + if (UI.modalID) + return; + UI.modalID = '#' + id; if (formID) { UI.modalForm = '#' + formID; @@ -223,6 +227,7 @@ var UI = { onCredentialsCancel: function UI_onCredentialsCancel(e) { $('#apikey').val(''); + ViewerController.credentialsCallback = null; }, @@ -234,6 +239,40 @@ var UI = { }, + hideRestricted: function UI_hideRestricted() { + this.hide('restricted'); + }, + + + showRestrictedOffer: function UI_showRestrictedOffer(count, onClick) { + var them = count == 1 ? 'it' : 'them'; + var text = count + ' bug' + (count == 1 ? '' : 's') + ' in this push could not be'; + text += ' loaded, most likely because ' + (count == 1 ? 'it is' : 'they are') + ' restricted.'; + text += ' If you have access to ' + them + ', you can load and mark ' + them; + text += ' with your api key.'; + + $('#restrictedText').text(text); + $('#restrictedButton').off('click.restricted').on('click.restricted', onClick); + $('#restrictedButton').show(); + this.show('restricted'); + }, + + + showRestrictedStatus: function UI_showRestrictedStatus(loaded, stillUnloaded) { + var text = 'Showing the ' + loaded + ' restricted bug' + (loaded == 1 ? '' : 's'); + text += ' from this push only. Everything else has been left out.'; + if (stillUnloaded.length > 0) { + text += stillUnloaded.length == 1 ? ' Bug ' : ' Bugs '; + text += stillUnloaded.join(', ') + ' could not be loaded with your api key,'; + text += ' and will still need marking by hand.'; + } + + $('#restrictedText').text(text); + $('#restrictedButton').off('click.restricted').hide(); + this.show('restricted'); + }, + + onAddBugSubmit: function UI_onAddBugSubmit(e) { var index = $('#addBugForm').attr('data-index'); ViewerController.onAddBug(parseInt(index), $('#loadBug').val()); diff --git a/bugherder/js/Viewer.js b/bugherder/js/Viewer.js index a93dcc6..710a505 100644 --- a/bugherder/js/Viewer.js +++ b/bugherder/js/Viewer.js @@ -19,6 +19,7 @@ var Viewer = { 'commentCheck': this.decorateWithRequired(this.onCommentCheckClick, indexBug, 'Comment'), 'resolveCheck': this.decorateWithRequired(this.onResolveCheckClick, indexBug, 'Resolve'), 'reopenCheck' : this.decorateWithRequired(this.onReopenCheckClick, indexBug, 'Reopen'), + 'securityReleaseCheck': this.decorateWithRequired(this.onSecurityReleaseCheckClick, indexBug, 'Security release'), 'viewhide' : this.decorateWithRequired(this.onViewHideClick, indexBug, 'View/Hide'), 'fileviewhide': this.decorateWithRequired(this.onFileViewHideClick, indexOnly, 'View/Hide files'), 'expandButton': this.onExpandButtonClick, @@ -37,10 +38,12 @@ var Viewer = { 'testsuite': this.decorateWithRequired(this.onTestsuiteChange, indexBug, 'Testsuite') }; + // init runs again when the steps are rebuilt, so don't stack a second handler set var self = this; - $('#viewerOutput').click(bindListener(self, clickListeners)); - $('#viewerOutput').on('input', bindListener(self, inputListeners)); - $('#viewerOutput').on('change', bindListener(self, changeListeners)); + $('#viewerOutput').off('.viewer'); + $('#viewerOutput').on('click.viewer', bindListener(self, clickListeners)); + $('#viewerOutput').on('input.viewer', bindListener(self, inputListeners)); + $('#viewerOutput').on('change.viewer', bindListener(self, changeListeners)); }, @@ -177,6 +180,8 @@ var Viewer = { // resolution). $('.'+bug+'Milestone').attr('disabled', !target.checked); + $('.'+bug+'securityReleasecheck').attr('checked', target.checked); + ViewerController.onResolveCheckClick(bug, target.checked); }, @@ -192,6 +197,14 @@ var Viewer = { }, + onSecurityReleaseCheckClick: function viewer_onSecurityReleaseCheckClick(index, bug, target) { + // Update all other instances of this bug + $('.'+bug+'securityReleasecheck').attr('checked', target.checked); + + ViewerController.onSecurityReleaseCheckClick(bug, target.checked); + }, + + onCommentCheckClick: function viewer_onCommentCheckClick(index, bug, target) { ViewerController.onCommentCheckClick(index, bug, target.checked); @@ -297,6 +310,11 @@ var Viewer = { }, + getSecurityReleaseCheckID: function viewer_getSecurityReleaseCheckID(cset, id) { + return cset + id + 'SecurityReleaseCheck'; + }, + + getRemoveButtonID: function viewer_getRemoveButtonID(cset, id) { return cset + id + 'Remove'; }, @@ -337,6 +355,11 @@ var Viewer = { makeMilestoneSelectHTML: function viewer_makeMilestoneSelectHTML(cset, index, id) { + var product = BugData.bugs[id].product; + // Milestones are already encoded by the time they reach bugInfo + if (!(product in ConfigurationData.milestones)) + return this.step.getMilestone(id); + var html = '