Create vendor agnostic JSON format between issue data sources and consumers #74

alancutter · 2016-10-13T01:12:29Z

This change rewrites Monorail issue data in terms of a JSON format defined in issue-lib.js.
Producers of Monorail issue data (just cz-monorail-issues) pass data through issue-lib.js before calling callbacks.
Consumers of issue data use the JSON format defined in issue-lib.js along with associated helper functions.

The purpose of this change is to allow consumers of issue data to consume GitHub issue data via a common JSON format without needed rewrites in future patches.

There are no behavioural changes in this patch.
This change built on top of #73.

BUG=#72

suzyh

I'd like to see the doc that you were putting together include some information about the structures involved here. For instance, a diagram showing the flow of data and what format the data is in at each point. I find the dashboard codebase difficult to navigate and, while much of that would be in the scope of a broader dashboard doc rather than your doc, diagrams would help with that.

suzyh · 2016-10-13T03:23:46Z

client/issue-lib.js


-if (window.Issue && window.IssueList) {
+if (window.Issue) {
  return;
 }



Nit: Since you're modifying this file, can you update the _reviewLevelMetadata query fields to use the casing now used in Monorail (Update-Daily, etc.)?

suzyh · 2016-10-13T03:30:57Z

client/cz-milestones-dash.html

-                var label = data[i].labels[j];
+          registerSource('cz-monorail-issues', {owner: user.email}, function(issues) {
+            for (var issue of issues) {
+              for (var label of issue.labels) {
                if (label.substring(0, 2).toLowerCase() == 'm-') {


I suspect this 'm-xx' label processing belongs in Issue.create as well.

M doesn't really make sense for issues other than monorail, nor is it used by any other dash. I'd rather leave this behaviour scoped to this particular dash.

alancutter

Added diagram to doc.

suzyh

lgtm with nits

suzyh · 2016-10-13T04:54:58Z

client/issue-lib.js

+      summary,
+      labels,
+      priority: undefined,
+      _lastUpdatedMS: Date.parse(lastUpdatedString),


It just occurred to me: Can we rely on Monorail and GitHub both providing lastUpdatedString in the same format, or at least both in formats that Date.parse will accept? Might need to add a precondition for Issue.create that specifies the format of the arguments (mainly lastUpdatedString, but possibly also labels).

Added assertion.

suzyh · 2016-10-13T04:55:47Z

client/cz-milestones-dash.html

-                var label = data[i].labels[j];
+          registerSource('cz-monorail-issues', {owner: user.email}, function(issues) {
+            for (var issue of issues) {
+              for (var label of issue.labels) {
                if (label.substring(0, 2).toLowerCase() == 'm-') {


suzyh · 2016-10-13T04:58:49Z

client/issue-lib.js

+  static reviewLevelWithBackoff(issue) {
+    var result = issue._reviewLevel;
+    if (result == _defaultReviewLevel) {
+      result += ' (P' + this.priority + ')';


Oh, missed this before. this.priority --> issue.priority?

This code assumed that this.priority would be defined because all Monorail issues had a Pri-X label. I don't think this will hold for GitHub. That might be something to change in future patches, however.

Oops, thanks!

Yes, I intend to deal with priority later when adding GitHub as a source.

alancutter · 2016-10-13T05:12:26Z

Changed description to use BUG= notation.

alancutter · 2016-10-13T05:16:41Z

Where is the doc?

See associated bug.

shans · 2016-10-17T04:11:43Z

This moves the abstraction from an IssueList to individual Issue objects. While I don't object to this, I'd like to see the change (and a justification) described in your design doc.

shans · 2016-10-17T04:20:23Z

In particular: everything is static on Issue now, including:
(1) things that don't look like they should be static (e.g. computational methods that take a single issue)
(2) functions that take lists of issues (which is quite weird to do as a static method on Issue).

Why the paradigm shift?

shans · 2016-10-17T04:23:03Z

client/issue-lib.js

+
+// This class uses C style programming with JSON objects (representing issues)
+// and static methods.
+// This is to maintain the ability to clone the object in chromez-behaviours.js.


You don't need to do this to allow objects to be cloned. You can either explicitly provide a clone() method, or serialize the representation automatically with json. Either way, methods on the object won't get in the way.

As I mentioned in the other comments, this is a major detail that needs to be included in design documentation. We should have been able to review this proposal before you went to the effort of coding it up.

True, this was a significant refactor to the Issue class that I should have included it in the design doc. I wouldn't have blocked coding it up on a proposal review though, a mechanical restructure of a single class takes <30 minutes while design review cycles are significantly longer than that.

I'll update design docs with refactor plans before starting them in future.

alancutter · 2016-10-17T05:46:28Z

Extended clone() instead of changing the paradigm of Issue. Added IssueList back. PTAL.

shans

(Please change the Issue constructor to avoid it being a copy constructor, unless there are more users than the clone() method. I don't think copy constructors are idiomatic JavaScript).

shans · 2016-10-17T06:21:14Z

client/cz-crbug-issues.html

      behaviors: [ChromeZBehaviors.AJAXBehavior],

      onResponse: function(data) {
        data = JSON.parse(data);
        if (data.items == undefined)
          return [];
-        return data.items.filter(a => a.ID !== "");
+        return new IssueList(data.items.filter(a => a.ID !== "").map(item => {


Consider moving the crbug-specific constructor into the issue library?

issues-lib is supposed to be vendor agnostic code, I don't think it makes sense to put Crbug specific code in there. This function performs the transformation from Crbug issue data into generic issue data. Would you prefer it live in crbug-lib.js?

shans · 2016-10-17T06:22:59Z

client/issues-lib.js

+class Issue {
+  constructor(params) {
+    // Copy constructor
+    if (params instanceof Issue) {


If this is only used by clone, then it's simpler to directly construct around a copy of the params in clone than to switch every time an issue is constructed.

Removed copy constructor.

alancutter · 2016-10-19T00:19:16Z

Ping @shans.

alancutter added 2 commits October 12, 2016 17:13

Consistently refer to Monorail as Monorail

75a0a4b

Remove unused include

805346b

alancutter changed the title ~~Create vender agnostic JSON protocol between issue data sources and consumers~~ Create vendor agnostic JSON protocol between issue data sources and consumers Oct 13, 2016

alancutter changed the title ~~Create vendor agnostic JSON protocol between issue data sources and consumers~~ Create vendor agnostic JSON format between issue data sources and consumers Oct 13, 2016

suzyh reviewed Oct 13, 2016

View reviewed changes

alancutter commented Oct 13, 2016

View reviewed changes

suzyh approved these changes Oct 13, 2016

View reviewed changes

alancutter added 7 commits October 13, 2016 16:45

crbug

7c6a31f

returnIssueList

001cdbe

Reword comment

96f051b

methods

97e5247

labels

ae04f4c

Casing

c6ede87

nan

00bcff2

alancutter force-pushed the returnIssueList branch from 9f39135 to 00bcff2 Compare October 13, 2016 05:50

alancutter mentioned this pull request Oct 14, 2016

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

Merged

shans reviewed Oct 17, 2016

View reviewed changes

alancutter added 2 commits October 17, 2016 16:41

Use clone method

7fccf32

method order

7bd4fe5

Minor fixes

7b62762

shans approved these changes Oct 17, 2016

View reviewed changes

alancutter added 2 commits October 18, 2016 11:10

Removed copy constructor

36c2198

Fix clone bug

7530f5c

shans merged commit 2128369 into shans:master Oct 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create vendor agnostic JSON format between issue data sources and consumers #74

Create vendor agnostic JSON format between issue data sources and consumers #74

alancutter commented Oct 13, 2016 •

edited

Loading

suzyh left a comment

suzyh Oct 13, 2016

alancutter Oct 13, 2016

suzyh Oct 13, 2016

alancutter Oct 13, 2016

suzyh Oct 13, 2016

alancutter left a comment

suzyh left a comment

suzyh Oct 13, 2016

alancutter Oct 13, 2016

suzyh Oct 13, 2016

suzyh Oct 13, 2016

alancutter Oct 13, 2016

alancutter commented Oct 13, 2016

alancutter commented Oct 13, 2016

shans commented Oct 17, 2016

shans commented Oct 17, 2016

shans Oct 17, 2016

alancutter Oct 17, 2016

alancutter commented Oct 17, 2016

shans left a comment

shans Oct 17, 2016

alancutter Oct 17, 2016

shans Oct 17, 2016

alancutter Oct 18, 2016

alancutter commented Oct 19, 2016

Create vendor agnostic JSON format between issue data sources and consumers #74

Create vendor agnostic JSON format between issue data sources and consumers #74

Conversation

alancutter commented Oct 13, 2016 • edited Loading

suzyh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alancutter left a comment

Choose a reason for hiding this comment

suzyh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alancutter commented Oct 13, 2016

alancutter commented Oct 13, 2016

shans commented Oct 17, 2016

shans commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alancutter commented Oct 17, 2016

shans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alancutter commented Oct 19, 2016

alancutter commented Oct 13, 2016 •

edited

Loading