-
Notifications
You must be signed in to change notification settings - Fork 13
Add an option to scrape all Bugzilla flags. #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Fixes mikeconley#26 There wasn't any UI code inside the onChange listener so I didn't make the needinfo setting visually subordinate to the all flags setting. Instead it is semantically subordinate.
Sorry for the delay - I hope to have this reviewed later this week. |
|
||
if (settings.allBugzillaFlags) { | ||
let flagCounts = {}; | ||
bugzillaData.result.result.requestee.map(f => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we need to use a map()
here, since we're not actually using the return value. This could probably be a forEach, or (preferable):
let flagCounts = {};
for (let flag of bugzillaData.result.result.requestee) {
if (!(f.type in flagCounts)) {
flagCounts[f.type] = 0;
}
flagCounts[f.type]++;
}
return flagCounts;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @chutten!
If you have the time, I have some minor suggestions here.
Mainly, I'm hoping we can have individual toggles for review
, feedback
, data-review
and needinfo
flags, so that users can pick and choose.
Do you think you'd have time to do that?
@@ -326,19 +326,31 @@ var MyQOnly = { | |||
if (bugzillaData.error) { | |||
throw new Error(`Bugzilla request failed: ${bugzillaData.error.message}`); | |||
} | |||
let reviewTotal = | |||
|
|||
if (settings.allBugzillaFlags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have the data, I guess we might as well do this count-up, regardless of whether or not this setting is set.
<div class="form-rows"> | ||
<input type="checkbox" id="bugzilla-allBugzillaFlags" data-setting="allBugzillaFlags"> | ||
<label for="bugzilla-allBugzillaFlags">Count all Bugzilla Flags (implies "Count open needinfos too")</label> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably, the flags we care about are:
review
feedback
data-review
needinfo
Can we have those as individual entries instead?
@mikeconley @chutten Any chance of moving this forward? I had the same problem of missing a feedback? because I basically rely on MyQOnly. If neither of you is able to continue working on this, I can give it a shot, but I wanted to nag just in case you totally forgot about this pull. |
@gcp Yeah, sorry, I haven't made any forward progress on this since March. Please feel free to take it over. |
Please do! |
ooh, this feature would be great! I can take this over the finish line |
...actually, I might just start a new patch |
Fixes #26
There wasn't any UI code inside the onChange listener so I didn't make the
needinfo setting visually subordinate to the all flags setting. Instead it is
semantically subordinate.