Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add "issue-sources" concept to config and make cz-component-dash use it #76

Merged
merged 11 commits into from
Oct 19, 2016
Merged

Add "issue-sources" concept to config and make cz-component-dash use it #76

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
116e860
Add issue-sources config and make cz-component-dash use it instead of…
alancutter Oct 14, 2016
ed816b2
Fix bugs
alancutter Oct 14, 2016
3fada69
Merge branch 'refs/heads/returnIssueList' into issueSources
alancutter Oct 17, 2016
873c6c6
Merge branch 'refs/heads/returnIssueList' into issueSources
alancutter Oct 17, 2016
efb5108
Merge branch 'refs/heads/returnIssueList' into issueSources
alancutter Oct 18, 2016
f640260
Don't return source element
alancutter Oct 18, 2016
6297f33
Merge branch 'refs/heads/returnIssueList' into issueSources
alancutter Oct 18, 2016
0aaca8e
Merge remote-tracking branch 'refs/remotes/origin/master' into return…
alancutter Oct 18, 2016
ea42a22
Merge branch 'refs/heads/returnIssueList' into issueSources
alancutter Oct 18, 2016
db46f33
Merge remote-tracking branch 'refs/remotes/origin/master' into issueS…
alancutter Oct 19, 2016
8e0f2e8
component name
alancutter Oct 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 69 additions & 47 deletions client/cz-component-dash.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
<link rel="import" href="../bower_components/paper-card/paper-card.html">
<link rel="import" href="../bower_components/paper-item/paper-item.html">
<link rel="import" href="../bower_components/paper-item/paper-item-body.html">
<script src="crbug-lib.js"></script>
<script src="issue-lib.js"></script>

<dom-module id="cz-component-dash">
Expand All @@ -19,7 +18,7 @@
align-content: flex-start;
}

.component {
.breakdown {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again with this one. "breakdown" isn't really more generic than "component", and is more confusing. I'd actually like you to change this back before committing.

display: flex;
flex-wrap: wrap;
padding: 6px;
Expand All @@ -44,16 +43,16 @@
text-decoration: none;
}
</style>
<a href="{{crbugLink(component.name)}}">
<paper-card heading="{{component.name}} Issues" id='card'>
<a href="{{queryURL(issueSource, sourceQuery)}}">
<paper-card heading="{{key}} Issues" id='card'>
<div class="card-content">
<div class='card-flex'>
<paper-item>
<paper-item-body two-line>
<div>Owned by us</div>
<div secondary class='component'>
<template is="dom-repeat" items="{{issues(component.teamsIssues, updateSLO)}}">
<a href="{{crbugLink(component.name, item.query)}}">
<div secondary class='breakdown'>
<template is="dom-repeat" items="{{summarize(issueBreakdown.team, updateSLO)}}">
<a href="{{queryURL(issueSource, sourceQuery, item.query)}}">
<span class="bubble" style="background-color: {{item.color}}">{{item.level}}</span>
<span class="summary">{{item.summary}}</span>
</a>
Expand All @@ -65,9 +64,9 @@
<paper-item>
<paper-item-body two-line>
<div>Owned by others</div>
<div secondary class='component'>
<template is="dom-repeat" items="{{issues(component.othersIssues, updateSLO)}}">
<a href="{{crbugLink(component.name, item.query)}}">
<div secondary class='breakdown'>
<template is="dom-repeat" items="{{summarize(issueBreakdown.others, updateSLO)}}">
<a href="{{queryURL(issueSource, sourceQuery, item.query)}}">
<span class="bubble" style="background-color: {{item.color}}">{{item.level}}</span>
<span class="summary">{{item.summary}}</span>
</a>
Expand All @@ -79,9 +78,9 @@
<paper-item>
<paper-item-body two-line>
<div>Unowned</div>
<div secondary class='component'>
<template is="dom-repeat" items="{{issues(component.unownedIssues, updateSLO)}}">
<a href="{{crbugLink(component.name, item.query)}}">
<div secondary class='breakdown'>
<template is="dom-repeat" items="{{summarize(issueBreakdown.unowned, updateSLO)}}">
<a href="{{queryURL(issueSource, sourceQuery, item.query)}}">
<span class="bubble" style="background-color: {{item.color}}">{{item.level}}</span>
<span class="summary">{{item.summary}}</span>
</a>
Expand All @@ -104,57 +103,80 @@
type: 'Object',
value: function() { return []; },
},
component: {
updateSLO: {
type: 'Object',
value: function() { return {}; },
value: function() { return null; },
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change? Also you don't need a function to initialize objects to null, just use value: null.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know we could use values directly, cool. I assume the functions are used for non-primitive value types then.

},
updateSLO: {
sourceQuery: {
type: 'Object',
value: function() { return null; },
},
issueSource: {
type: 'Object',
value: function() { return null; },
},
issueBreakdown: {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you reorder this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea TBH. :/

type: 'Object',
value: function() { return {}; },
value: function() { return {}; }
}
},

attached: function() {
registerSource('cz-config', 'users', function(users) {
registerSource('cz-config', 'users', users => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this is an example of an unrelated change that I don't mind seeing in a review. It's well contained, improves readability, and doesn't force me to chase changes all over the file.)

this.set('users', users.map(userData => userData['email']));
}.bind(this));
registerSource('cz-config', 'components', function(components) {
this.set('component', {name: components[this.key]});
this.updateComponentIssues(this.component)
}.bind(this));
registerSource('cz-config', 'updateSLO', function(updateSLO) {
});
registerSource('cz-config', 'updateSLO', updateSLO => {
this.set('updateSLO', updateSLO);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.updateSLO = updateSLO. Also I think it's fine to put it all on one line now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we should use this.set() when the property is used in the template otherwise the template is not guaranteed to react to the change. Is this an incorrect assumption?

}.bind(this));
});
registerSource('cz-config', 'issue-sources', issueSources => {
this.updateIssueSource(issueSources[this.key]);
});
},

issues: function(issues, updateSLO) {
summarize: function(issues, updateSLO) {
if (!issues || !updateSLO) {
return [];
}
return Issue.summarizeList(issues, updateSLO);
},

updateComponentIssues: function(component) {
registerSource('cz-crbug-issues', {component: component.name}, function(issues) {
var teamsIssueList = [];
var othersIssueList = [];
var unownedIssueList = [];
for (var issue of issues) {
if (issue.owner == null) {
unownedIssueList.push(issue);
} else if (this.users.indexOf(issue.owner) === -1) {
othersIssueList.push(issue);
} else {
teamsIssueList.push(issue);
}
updateIssueSource: function({tag, query}) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) splitting this out does not improve readability, but does destroy locality of effect

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a remnant of when I was storing a separate issueSource property that made this split necessary. It is no longer necessary and can be inlined again.

registerSource(tag, query, issues => {
this.updateIssues(issues);
}).then(sourceElement => {
this.set('issueSource', sourceElement);
this.set('sourceQuery', query);
});
},

updateIssues: function(issues) {
var team = [];
var others = [];
var unowned = [];
for (var issue of issues) {
if (issue.owner == null) {
unowned.push(issue);
} else if (this.users.indexOf(issue.owner) === -1) {
others.push(issue);
} else {
team.push(issue);
}
this.set('component.teamsIssues', teamsIssueList);
this.set('component.othersIssues', othersIssueList);
this.set('component.unownedIssues', unownedIssueList);
}.bind(this));
}
this.set('issueBreakdown.team', team);
this.set('issueBreakdown.others', others);
this.set('issueBreakdown.unowned', unowned);
},

crbugLink: function(component, query) {
var componentQuery = clone(query || {});
componentQuery.component = component;
return Crbug.queryURL(componentQuery);
queryURL: function(issueSource, sourceQuery, itemQuery) {
if (!issueSource || !sourceQuery) {
return '';
}
var query = sourceQuery;
if (itemQuery) {
query = clone(query);
Object.assign(query, itemQuery);
}
return this.issueSource.queryURL(query);
},
});
</script>
Expand Down
7 changes: 6 additions & 1 deletion client/cz-crbug-issues.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<script src="../bower_components/papaparse/papaparse.min.js"></script>
<script src="chromez-behaviors.js"></script>
<script src="issue-lib.js"></script>
<script src="crbug-lib.js"></script>

<dom-module id="cz-crbug-issues">

Expand Down Expand Up @@ -45,7 +46,11 @@
q: Crbug.queryString(query),
};
this.addCallbackToQuery(query, callback, params);
}
},

queryURL: function(query) {
return Crbug.queryURL(query);
},
});
</script>
</dom-module>
5 changes: 4 additions & 1 deletion client/cz-registry.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@
}.bind(this));
}

return this.registeredSources[type].then(source => source.registerQuery(query, callback));
return this.registeredSources[type].then(source => {
source.registerQuery(query, callback);
return source;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drawing attention to this change.
Is it weird to return a promise to the source element from registerSource()? I added this to support being able to call queryURL() on the issue source element to get an issue vendor specific URL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets used again in #77 where the dash asks the issue source to give its interpretation of a username from the user config as it differs between Crbug and Github.

});
},
});
</script>
Expand Down
8 changes: 7 additions & 1 deletion configs/animations-ave.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
},
"dashes": [
"cz-clock-dash",
"cz-component-dash(animation)",
"cz-component-dash(Blink Animations)",
"cz-issue-priority-dash",
"cz-regression-dash",
"cz-review-latency-dash(false)",
Expand All @@ -41,5 +41,11 @@
"repo": "web-animations/web-animations-next",
"shields": ["travis", "github/issues-pr", "github/issues"]
}
},
"issue-sources": {
"Blink Animations": {
"tag": "cz-crbug-issues",
"query": {"component": "Blink>Animation"}
}
}
}