Final project feedback

authentication.js

@@ -31,7 +31,7 @@ module.exports = {
     passport.use('local-login', new LocalStrategy({
         usernameField: 'username',
         passwordField: 'password',
-        passReqToCallback : true
+        passReqToCallback: true
     },
     function(req, username, password, done) {
       User.findOne({ 'username' :  username }, function(err, user) {
@@ -175,7 +175,7 @@ module.exports = {
         email: req.user.email,
         orgs: req.user.orgs,
         isAdmin: req.user.isAdmin,
-        _id: req.user._id  
+        _id: req.user._id
       });
     } else {
       res.json({
@@ -378,7 +378,7 @@ module.exports = {
 };
 
 /**
- * A generic route callback type often called by authentication methods.
+ * A generic route callback type often called by authentication methods. <-- does the function associated w/ this header exist?
  * @callback nextRoute
  * @param {Object} req An Object representing the http request.
  * @param {Object} res An Object representing the http response.

models/userModel.js

@@ -72,12 +72,12 @@ User.statics.generateHash = function(password) {
 User.methods.validPassword = function(password) {
   var hash = this.password;
   return new Promise(function(resolve, reject) {
-    console.log(password);
+    console.log(password); // logging passwords is probably not a good idea!
     console.log(hash);
     bcrypt.compare(password, hash, function(err, res) {
       resolve(res);
     });
   });
 };
 
-module.exports = mongoose.model("users", User);
+module.exports = mongoose.model("users", User);

public/javascripts/client.js

@@ -5,7 +5,7 @@ var app = angular.module('ledger', [
     'ngTouch',
     'nvd3',
     'ui.bootstrap',
-    'smart-table' 
+    'smart-table'
   ]).config([
     '$routeProvider',
     '$locationProvider',
@@ -122,6 +122,8 @@ var app = angular.module('ledger', [
     $locationProvider.html5Mode(true);
 
 }]).controller('NavigationCtrl', function($scope, $location) {
+  // why doesn't the navigation controller get its own file?
+  // I think it could, for consistency (unless there's a good reason it needs to be here)
   $scope.isActive = function (viewLocation) {
     return viewLocation === $location.path();
   };

public/javascripts/controllers/AccountController.js

@@ -76,11 +76,11 @@ app.controller('AccountController', function($uibModal, AuthService) {
       helpBlock: ''
     },
     validate: function() {
-      var regex = new RegExp("^[a-zA-Z0-9]+$");
+      var regex = new RegExp("^[a-zA-Z0-9]+$"); // give regex a more semantic variable name? e.g. var validPasswordRegex = ...
       if (typeof this.value !== 'string' ||
           !regex.test(this.value)) {
         this.validation.isValid = 'invalid';
-        this.validation.helpBlock = 
+        this.validation.helpBlock =
             'Password must only use letters and numbers and cannot be empty';
       } else {
         this.validation.isValid = 'valid';
@@ -171,7 +171,7 @@ app.controller('AccountController', function($uibModal, AuthService) {
           });
         } else {
           AccCtrl.currentPassword.validation.isValid = 'invalid';
-          AccCtrl.currentPassword.validation.helpBlock = 
+          AccCtrl.currentPassword.validation.helpBlock =
             'Incorrect password';
         }
         if (res.isSuccessful) { AccCtrl.closeEditPassword(); }
@@ -196,4 +196,4 @@ app.controller('AccountController', function($uibModal, AuthService) {
     };
     this.confirmNewPassword.value = '';
   };
-});
+});

public/javascripts/controllers/DashboardController.js

@@ -7,12 +7,7 @@ app.controller('DashboardController', function($scope, RequestService, AuthServi
 		isCollapsed: true
 	};
 
-
-	console.log("AuthService.currentUser");
-	console.log(AuthService.currentUser);
 	RequestService.getRequests(AuthService.currentUser._id).then(function(requests) {
-		console.log("requests");
-		console.log(requests);
 		DashCtrl.requests = requests;
 	});
 
@@ -32,14 +27,12 @@ app.controller('DashboardController', function($scope, RequestService, AuthServi
 
 
     RequestService.editRequest(targetRequest).then(function(success){
-      if(success){
-        console.log("Modification Success");
-
-      }else{
-        alert("Modification Failure");
+      if (success) {
+        console.log("Modification Success"); // do you really need to log this?
+      } else {
+        alert("Modification Failure"); // Is an alert the right choice? I think you guys mostly use in-page error/validation messages... be consistent?
       }
-    })
-
+    });
   };
 
   this.cancel = function(index, ans){
@@ -48,15 +41,13 @@ app.controller('DashboardController', function($scope, RequestService, AuthServi
     targetRequest.isDecided = true;
     targetRequest.isActive = false;
 
-    console.log("targetRequest");
-    console.log(targetRequest);
     RequestService.editRequest(targetRequest).then(function(success){
-      if(success){
+      if (success) {
         console.log("Modification Success");
-      }else{
+      } else {
         alert("Modification Failure");
       }
     })
   };
 
-});
+});

public/javascripts/controllers/LoginController.js

@@ -21,4 +21,4 @@ app.controller('LoginController', function(AuthService) {
   this.dismissAlert = function(index) {
     this.alerts.splice(index, 1);
   };
-});
+});

public/javascripts/controllers/MainController.js

@@ -13,7 +13,7 @@ app.controller('MainController', function($scope, $location, AuthService) {
   this.onPermissionsUpdate = function(user) {
     MainCtrl.user = user;
     AuthService.currentUser = user;
-
+    // This is really clean
     var navRoutes = [];
     var authRoutes = [{
       name: 'Login',
@@ -60,4 +60,4 @@ app.controller('MainController', function($scope, $location, AuthService) {
 
   AuthService.registerPermissionsUpdateCallback(this.onPermissionsUpdate);
 
-});
+});

public/javascripts/controllers/ManageController.js

@@ -36,6 +36,7 @@ app.controller('ManageController', function(AuthService, OrgService, RequestServ
       }
     });
   };
+
   this.updateOrgs = function() {
     for (id in this.orgs) {
       this.orgs[id].ownersList = this.orgs[id].owners.map(function(owner) {
@@ -166,7 +167,6 @@ app.controller('ManageController', function(AuthService, OrgService, RequestServ
     }
   };
 
-
   this.resolveUser = function(index, isApproved) {
     AuthService.resolveUser(this.pendingUsers[index], isApproved, this.pendingUsers[index].email).then(function(response) {
       MngCtrl.alert.isActive = true;
@@ -206,9 +206,7 @@ app.controller('ManageController', function(AuthService, OrgService, RequestServ
     MngCtrl.updateOrgs();
   });
 
-
-
- this.comment = {
+  this.comment = {
     value: '',
     validation: {
       isValid: 'empty',
@@ -223,40 +221,27 @@ app.controller('ManageController', function(AuthService, OrgService, RequestServ
         this.validation.isValid = 'valid';
       }
     }
-  }
-
-  this.resolveFunRequest = function(index, ans){
-    // this.isCollapsed = !this.isCollapsed;
-    // console.log("this.isCollapsed");
-    // console.log(this.isCollapsed);
-
-
-    console.log(this.comment.value);
+  };
 
+  this.resolveFunRequest = function(index, ans){ // again, semantic variable names -- what's a "fun request"?
     var targetRequest = MngCtrl.pendingFundRequests.splice(index, 1);
 
-
     targetRequest = targetRequest[0]
     targetRequest.isDecided = true;
     targetRequest.comment = MngCtrl.comment.value
-    if(ans){
+    if (ans) {
       targetRequest.isApproved = true;
-    }else{
+    } else {
       targetRequest.isApproved = false;
     }
-    console.log("targetRequest");
-    console.log(targetRequest);
 
     RequestService.editRequest(targetRequest).then(function(success){
-      if(success){
-        console.log("Modification Success");
+      if (success) {
         MngCtrl.comment.value = "";
         MngCtrl.isCollapsed = true;
-      }else{
+      } else {
         alert("Modification Failure");
       }
     })
   };
-
-
-});
+});

public/javascripts/controllers/OrgController.js

@@ -1,8 +1,9 @@
 // public/javascripts/controllers/OrgController.js
 app.controller('OrgController', function($routeParams, AuthService,
-    OrgService, $location, $window, $scope) {
+    OrgService, $location, $window, $scope) { // I've usually seen the parameters to an Angular controller alphabetized -- common convention
 
   var OrgCtrl = this;
+  // vv make sure to clear out hardcoded dummy data before you deploy
   this.transfers = [
     { type: 'budget', from: 'SG', to: 'CORe', justification: '', value: 4 },
     { type: 'transfer', from: 'SG', to: 'CORe' , justification: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Maecenas cursus venenatis arcu eget laoreet. Pellentesque in consectetur dolor. Nulla facilisi. Proin commodo auctor condimentum. Donec eu mauris id neque dapibus placerat. Mauris luctus urna sit amet mi iaculis eleifend. Nunc id tellus risus. Ut mollis lorem luctus, convallis eros sit amet, eleifend nulla. Integer elit tortor, vulputate quis auctor id, elementum et lectus.', value: 50 }
@@ -298,4 +299,4 @@ app.controller('OrgController', function($routeParams, AuthService,
       });
     })
   });
-})
+})

public/javascripts/controllers/RecordController.js

@@ -5,6 +5,7 @@ app.controller('RecordController', function(RecordService, RequestService,
   this.categories =
       ['Food', 'Consumable Supplies', 'Long Term Supplies', 'Service/Events'];
 
+  // These constructors are identical to the Field function in requestController -- could you modularize into a shared utils file, like you did serverside? or maybe a service?
   function Field(initialValue) {
     this.value = initialValue;
     this.isValidated = false;

public/javascripts/controllers/RegisterController.js

@@ -28,7 +28,7 @@ app.controller('RegisterController', function(AuthService) {
     validate: function() {
       if (!this.value || this.value.length < 3) {
         this.validation.isValid = 'invalid';
-        this.validation.helpBlock = 
+        this.validation.helpBlock =
             'Username must be at least 3 characters long';
       } else {
         var username = this;
@@ -82,7 +82,7 @@ app.controller('RegisterController', function(AuthService) {
       var regex = new RegExp("^[a-zA-Z0-9]+$");
       if (!this.value || !regex.test(this.value)) {
         this.validation.isValid = 'invalid';
-        this.validation.helpBlock = 
+        this.validation.helpBlock =
             'Password must only use letters and numbers';
       } else {
         this.validation.isValid = 'valid';
@@ -97,7 +97,7 @@ app.controller('RegisterController', function(AuthService) {
   };
 
   this.submitRegistrationForm = function() {
-    var RegCtrl = this;
+    var RegCtrl = this; // isn't this done globally? (this file, line 15)
     this.username.validate();
     this.password.validate();
     this.name.validate();
@@ -132,4 +132,4 @@ app.controller('RegisterController', function(AuthService) {
       });
     }
   }
-});
+});

public/javascripts/controllers/SearchController.js

@@ -8,7 +8,7 @@ app.controller('SearchController', function($scope, OrgService,
     }
   });
 
-  $scope.req = true;
+  $scope.req = true; // why not use full request/record instead of req/rec? Easier to read & maintain, in my opinion.
   $scope.rec = true;
 
   $scope.rowCollection = [];
@@ -32,4 +32,4 @@ app.controller('SearchController', function($scope, OrgService,
     }
   };
 
-});
+});

public/javascripts/directives/OrgListingDirective.js

@@ -40,10 +40,10 @@ app.directive('listing', function ($compile) {
     link: function (scope, element, attrs) {
       scope.$watch('children', function() {
         if (scope.children[scope.parent].length > 0) {
-          element.append("<listing ng-repeat='child in children[parent]' directory='directory' parent='child' children='children'></listing>");
+          element.append("<listing ng-repeat='child in children[parent]' directory='directory' parent='child' children='children'></listing>"); // recursion! This is cool.
           $compile(element.contents())(scope)
         }
       })
     }
   }
-})
+})

public/javascripts/services/AuthService.js

@@ -98,6 +98,7 @@ app.service('AuthService', function($http, $q, $rootScope, $location) {
     var ifPermissionPassed = false;
 
     angular.forEach(validRoles, function (role) {
+      // This is really clean
       switch (role) {
         case roles.UNAUTH:
           if (!user.isAuthenticated) {

public/javascripts/services/OrgService.js

@@ -72,6 +72,18 @@ app.service('OrgService', function($http, $q) {
       return deferred.resolve(response.data);
     });
     return deferred.promise;
+    /*
+    Not 100% sure, but you might be able to just
+    return $http.post('/api/createTransfer', {
+      org: org,
+      to: to,
+      from: from,
+      value: value,
+      justification: justification
+    });
+
+    ...$http.post is a promise, as evidenced by its .then method :)
+    */
   };
 
-});
+});