Merge lp:~bac/juju-gui/bug-1246685 into lp:juju-gui/experimental

[Brad Crittenden]

Fix name/email address parsing for commits.

https://codereview.appspot.com/20170044/

[Brad Crittenden (bac)]

Reviewers: mp+193450_code.launchpad.net,

Message:
Please take a look.

Description:
Fix name/email address parsing for commits.

https://code.launchpad.net/~bac/juju-gui/bug-1246685/+merge/193450

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/20170044/

Affected files (+80, -39 lines):
   A [revision details]
   M app/models/bundle.js
   M test/data/browserbundle.json
   M test/test_model_bundle.js

[Brad Crittenden (bac)]

Author comments

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js
File app/models/bundle.js (left):

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js#oldcode67
app/models/bundle.js:67: /**
Took Jeff's advice and moved back into the class for easy testing
access.

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js
File app/models/bundle.js (right):

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js#newcode69
app/models/bundle.js:69: parseNameEmail: function(author) {
Actually we only care about the name as the currently don't display the
email address. However to match the data collected for charms I chose
to go ahead and squirrel it away.

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js#newcode81
app/models/bundle.js:81: @method extractRecentCommits
I see I didn't document the changes arg. Will do so.

https://codereview.appspot.com/20170044/diff/1/test/data/browserbundle.json
File test/data/browserbundle.json (right):

https://codereview.appspot.com/20170044/diff/1/test/data/browserbundle.json#newcode9
test/data/browserbundle.json:9: "Brad Crittenden <email address hidden>"
Not using a deliverable email address is preferred.

https://codereview.appspot.com/20170044/diff/1/test/data/browserbundle.json#newcode18
test/data/browserbundle.json:18: "Jorge O. O'Castro
<email address hidden>",
Took the liberty of making Jorge Irish to catch more special characters.

https://codereview.appspot.com/20170044/

[Richard Harding (rharding)]

code looks ok, comments below. Will QA now.

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js
File app/models/bundle.js (right):

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js#newcode75
app/models/bundle.js:75: },
space between functions

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js#newcode88
app/models/bundle.js:88: Y.Array.each(changes, function(change) {
if this is an Array, can we just use changes.forEach instead and not
need the YUI module for it?

https://codereview.appspot.com/20170044/diff/1/test/data/browserbundle.json
File test/data/browserbundle.json (right):

https://codereview.appspot.com/20170044/diff/1/test/data/browserbundle.json#newcode18
test/data/browserbundle.json:18: "Jorge O. O'Castro
<email address hidden>",
On 2013/10/31 17:26:28, bac wrote:
> Took the liberty of making Jorge Irish to catch more special
characters.

well played sir

https://codereview.appspot.com/20170044/diff/1/test/test_model_bundle.js
File test/test_model_bundle.js (right):

https://codereview.appspot.com/20170044/diff/1/test/test_model_bundle.js#newcode204
test/test_model_bundle.js:204: var parts = instance.parseNameEmail(
what do you think of naming this more to match the data of
parseAuthorInformation or something?

https://codereview.appspot.com/20170044/

[Richard Harding (rharding)]

LGTM QA'ok and works with Jorge's bundle now. Thanks for the quick fix!

https://codereview.appspot.com/20170044/

[Brad Crittenden (bac)]

*** Submitted:

Fix name/email address parsing for commits.

R=rharding
CC=
https://codereview.appspot.com/20170044

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js
File app/models/bundle.js (right):

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js#newcode75
app/models/bundle.js:75: },
On 2013/10/31 17:41:09, rharding wrote:
> space between functions

I don't know what you mean here.

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js#newcode81
app/models/bundle.js:81: @method extractRecentCommits
On 2013/10/31 17:26:28, bac wrote:
> I see I didn't document the changes arg. Will do so.

Done.

https://codereview.appspot.com/20170044/diff/1/app/models/bundle.js#newcode88
app/models/bundle.js:88: Y.Array.each(changes, function(change) {
On 2013/10/31 17:41:09, rharding wrote:
> if this is an Array, can we just use changes.forEach instead and not
need the
> YUI module for it?

Done.

https://codereview.appspot.com/20170044/diff/1/test/test_model_bundle.js
File test/test_model_bundle.js (right):

https://codereview.appspot.com/20170044/diff/1/test/test_model_bundle.js#newcode204
test/test_model_bundle.js:204: var parts = instance.parseNameEmail(
It seems longer and harder to read and not as obvious. I guess I'm -3.
:)

https://codereview.appspot.com/20170044/

=== modified file 'app/models/bundle.js'
--- app/models/bundle.js	2013-10-29 22:51:19 +0000
+++ app/models/bundle.js	2013-10-31 16:20:30 +0000
@@ -36,35 +36,6 @@
   var models = Y.namespace('juju.models');
 
   /**
-
-   Extract the recent commits into a format we can use nicely.  Output matches
-   the analogous function for charms.
-
-   @method extractRecentCommits
-   @return {array} Commit objects.
-  */
-  var extractRecentCommits = function(changes) {
-    var commits = [];
-
-    if (changes) {
-      Y.Array.each(changes, function(change) {
-
-        var author_parts = /^([\s\w]+?) <(.+)>$/.exec(change.authors[0]);
-        var date = new Date(change.created * 1000);
-        commits.push({
-          author: {
-            name: author_parts[1],
-            email: author_parts[2]
-          },
-          date: date,
-          message: change.message,
-          revno: change.revno
-        });
-      });
-    }
-    return commits;
-  };
-  /**
    * Model to represent the Bundles from the Charmworld API.
    *
    * @class Bundle
@@ -82,8 +53,55 @@
     initializer: function(cfg) {
       this.loaded = false;
       this.on('load', function() { this.loaded = true; });
+    },
+
+    /**
+
+     Parse a combined name-email string of the form
+     "Full Name <email.example.com>".
+
+     Any string that is not parseable has the whole string used as the name
+     and the email set to 'n/a'.
+
+     @method parseNameEmail
+     @return {array} [name, emailAddress]
+    */
+    parseNameEmail: function(author) {
+      var parts = /^([^<]+?) <(.+)>$/.exec(author);
+      if (Y.Lang.isNull(parts)) {
+        parts = [null, author, 'n/a'];
+      }
+      return [parts[1], parts[2]];
+    },
+    /**
+
+     Extract the recent commits into a format we can use nicely.  Output matches
+     the analogous function for charms.
+
+     @method extractRecentCommits
+     @return {array} Commit objects.
+    */
+    extractRecentCommits: function(changes) {
+      var commits = [];
+
+      if (changes) {
+        Y.Array.each(changes, function(change) {
+
+          var author_parts = this.parseNameEmail(change.authors[0]);
+          var date = new Date(change.created * 1000);
+          commits.push({
+            author: {
+              name: author_parts[0],
+              email: author_parts[1]
+            },
+            date: date,
+            message: change.message,
+            revno: change.revno
+          });
+        }, this);
+      }
+      return commits;
     }
-
   }, {
     /**
       Static to indicate the type of entity so that other code
@@ -204,14 +222,14 @@
       recentCommits: {
         /**
          * Return the commits of the charm in a format we can live with from
-         * the source code data provided by the api.
+         * the source code data provided by the API.
          *
          * @method recentCommits.valueFn
          *
          */
         valueFn: function() {
           var changes = this.get('changes');
-          return extractRecentCommits(changes);
+          return this.extractRecentCommits(changes);
         }
       }
     }

=== modified file 'test/data/browserbundle.json'
--- test/data/browserbundle.json	2013-10-29 19:38:27 +0000
+++ test/data/browserbundle.json	2013-10-31 16:20:30 +0000
@@ -6,27 +6,28 @@
   "changes": [
     {
       "authors": [
-        "Brad Crittenden <[e-mail]>"
+        "Brad Crittenden <[e-mail]>"
       ],
-      "committer": "Brad Crittenden <[e-mail]>",
+      "committer": "Brad Crittenden <[e-mail]>",
       "created": 1378218161.439,
       "message": "Correct icon",
       "revno": 3
     },
     {
       "authors": [
-        "Brad Crittenden <[e-mail]>"
+        "Jorge O. O'Castro <[e-mail]>",
+        "Brad Crittenden <[e-mail]>"
       ],
-      "committer": "Brad Crittenden <[e-mail]>",
+      "committer": "Brad Crittenden <[e-mail]>",
       "created": 1377718535.465,
       "message": "Added README and icon",
       "revno": 2
     },
     {
       "authors": [
-        "Brad Crittenden <[e-mail]>"
+        "Brad Crittenden <[e-mail]>"
       ],
-      "committer": "Brad Crittenden <[e-mail]>",
+      "committer": "Brad Crittenden <[e-mail]>",
       "created": 1375449744.902,
       "message": "Initial checking of working config.",
       "revno": 1

=== modified file 'test/test_model_bundle.js'
--- test/test_model_bundle.js	2013-10-29 22:51:19 +0000
+++ test/test_model_bundle.js	2013-10-31 16:20:30 +0000
@@ -168,7 +168,14 @@
     instance = new models.Bundle(data);
     var commits = instance.get('recentCommits');
     assert.equal('Brad Crittenden', commits[0].author.name);
-    assert.equal('[e-mail]', commits[0].author.email);
+    assert.equal('[e-mail]', commits[0].author.email);
+  });
+
+  it('only the first author is shown', function() {
+    instance = new models.Bundle(data);
+    var commits = instance.get('recentCommits');
+    assert.equal('Jorge O. O\'Castro', commits[1].author.name);
+    assert.equal('[e-mail]', commits[1].author.email);
   });
 
   it('has the revnos in reverse order', function() {
@@ -192,4 +199,17 @@
     assert.equal('Correct icon', commits[0].message);
   });
 
+  it('parses full name-email string', function() {
+    instance = new models.Bundle();
+    var parts = instance.parseNameEmail(
+        'Jorge O. O\'Castro <[e-mail]>');
+    assert.deepEqual(['Jorge O. O\'Castro', '[e-mail]'], parts);
+  });
+
+  it('gracefully handles unparseable name-email', function() {
+    instance = new models.Bundle();
+    var parts = instance.parseNameEmail('Jorge O. Castro');
+    assert.deepEqual(['Jorge O. Castro', 'n/a'], parts);
+  });
+
 });