RedirectDb: Extract updateProperty, minor updates

These updates help make the error messages more consistent and meaningful.
mbland · May 2, 2017 · e5e980b · e5e980b
1 parent eb398d5
commit e5e980b
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 21 deletions.
diff --git a/lib/redirect-db.js b/lib/redirect-db.js
@@ -28,10 +28,11 @@ RedirectDb.prototype.createRedirect = function(url, location, user) {
         ' to be owned by ' + user + ': ' + err)
     })
     .then(function() {
-      return db.client.addUrlToOwner(user, url).catch(function(err) {
-        throw new Error('redirection created for ' + url +
-          ', but failed to add to list for user ' + user + ': ' + err)
-      })
+      return db.client.addUrlToOwner(user, url)
+        .catch(function(err) {
+          throw new Error('redirection created for ' + url +
+            ', but failed to add to list for user ' + user + ': ' + err)
+        })
     })
 }
 
@@ -49,21 +50,30 @@ RedirectDb.prototype.getOwnedRedirects = function(user) {
 }
 
 function checkOwnership(client, url, user) {
-  return client.getRedirect(url).then(function(urlData) {
-    if (!urlData) {
-      throw new Error('no redirection exists for ' + url)
-    } else if (urlData.owner !== user) {
-      throw new Error('redirection for ' + url + ' is owned by ' +
-        urlData.owner)
-    }
-  })
+  return client.getRedirect(url)
+    .then(function(urlData) {
+      if (!urlData) {
+        throw new Error('no redirection exists for ' + url)
+      } else if (urlData.owner !== user) {
+        throw new Error('redirection for ' + url + ' is owned by ' +
+          urlData.owner)
+      }
+    })
+}
+
+function updateProperty(client, url, property, value) {
+  return client.updateProperty(url, property, value)
+    .catch(function(err) {
+      throw new Error('failed to update ' + property + ' of ' + url + ' to ' +
+        value + ': ' + err)
+    })
 }
 
 RedirectDb.prototype.changeOwner = function(url, user, newOwner) {
   var db = this
   return checkOwnership(db.client, url, user)
     .then(function() {
-      return db.client.updateProperty(url, 'owner', newOwner)
+      return updateProperty(db.client, url, 'owner', newOwner)
     })
     .then(function() {
       var errPrefix = 'changed ownership of ' + url + ' from ' + user +
@@ -85,11 +95,7 @@ RedirectDb.prototype.updateLocation = function(url, user, newLocation) {
   var db = this
   return checkOwnership(db.client, url, user)
     .then(function() {
-      return db.client.updateProperty(url, 'location', newLocation)
-        .catch(function(err) {
-          throw new Error('failed to update location of ' + url + ' to ' +
-            newLocation + ': ' + err)
-        })
+      return updateProperty(db.client, url, 'location', newLocation)
     })
 }
 
@@ -105,5 +111,10 @@ RedirectDb.prototype.deleteRedirection = function(url, user) {
     })
     .then(function() {
       return db.client.removeUrlFromOwner(user, url)
+        .catch(function(err) {
+          throw new Error('deleted redirection from ' + url +
+            ', but failed to remove URL from the owner\'s list for ' + user +
+            ': ' + err)
+        })
     })
 }
diff --git a/tests/redirect-db-test.js b/tests/redirect-db-test.js
@@ -190,7 +190,8 @@ describe('RedirectDb', function() {
             [url, name, value].join(' ')))
         })
       return redirectDb.changeOwner('/foo', 'msb', 'mbland')
-        .should.be.rejectedWith(Error, 'forced error for /foo owner mbland')
+        .should.be.rejectedWith(Error, 'failed to update owner of /foo ' +
+          'to mbland: Error: forced error for /foo owner mbland')
     })
 
     it('fails if adding to the new owner\'s URL list fails', function() {
@@ -266,7 +267,8 @@ describe('RedirectDb', function() {
             [url, name, value].join(' ')))
         })
       return redirectDb.updateLocation('/foo', 'mbland', '/baz')
-        .should.be.rejectedWith(Error, 'forced error for /foo location /baz')
+        .should.be.rejectedWith(Error, 'failed to update location of /foo ' +
+          'to /baz: Error: forced error for /foo location /baz')
     })
   })
 
@@ -320,7 +322,9 @@ describe('RedirectDb', function() {
         })
 
       return redirectDb.deleteRedirection('/foo', 'mbland')
-        .should.be.rejectedWith(Error, 'forced error for mbland /foo')
+        .should.be.rejectedWith(Error, 'deleted redirection from /foo, ' +
+          'but failed to remove URL from the owner\'s list for mbland: ' +
+          'Error: forced error for mbland /foo')
     })
   })
 })