Project client

[lomamech]

Updated part client of unit project and adding the test end to end

@jniles @DedrickEnc

[jniles]

You can use the $http service to set querystrings for you via a configuration object. Here is the relevant documenation.

Instead of creating a whole new method, you should just use service.read! Something like this:

function read(id, params) {
   var url = (id) ? '/projects/' + id : '/projects';
   return $http.get(url, { params : params });
}

// in your controller
ProjectService.read(null, { complete : 1 })
.then(function (projects) {
  console.log('The complete projects:', projects);
});

AngularJS should automatically serialize the parameters into a query string for you.

[jniles]

@lomamech, I've completed an initial review. Somethings to remember:

1. All of the server routes must have req, res, next as parameters. Using next() is the best practice for handling errors in your routes. In your get_snis_zs route, you do not have any error handling at all.
2. According to Travis, you have an error in your SQL syntax. (In MySQL, you cannot have comments like "--comment".... there must be a space in between the "--" and the "comment"). Did you actually build the test database before submitting this PR?

Thank you for using the "@jniles" syntax. Let me know when you've look into these errors.

[lomamech]

@jniles I'm looking at thes errors and I start simultaneously correction

[lomamech]

@jniles @DedrickEnc I just correct errors but also to make improvements,

[jniles]

@lomamech, this is good to see. Your integration tests do not pass. Here are your results.

I'll review the code when the tests pass.

[jniles]

Why did you change this?

[lomamech]

@jniles just to not have a big difference with the other function

[jniles]

That's a good intention, but the reason we cannot call the function "delete" is because delete is a javascript reserved word. To make the API clear, the API should still be called service.delete.

[jniles]

var service = this.

[jniles]

@lomamech, the end to end tests pass for me on master, but do not pass on your branch. I've uploaded your results.

Please, please, run end to end tests and integration tests before submitting a PR.

[lomamech]

@jniles end to end tests and integration tests run very well

[jniles]

14 passing, 6 failing end to end tests.

[lomamech]

@jniles 6 test end to end had fails because the data format sended by ProjectService

[jniles]

You can delete this entire section (<div class="report visible-print>") - it is no longer used and is bad practice. It also slows down the loading of the module.

[jniles]

This is still not fixed. Please delete this entire <div>.

[jniles]

The proper way to make a bootstrap checkbox input is to use the "checkbox" class. Here is the documentation. You should be using the "checkbox" class on a <div> and wrapping the <label> element around the checkbox.

[jniles]

@lomamech, these are very nice tests. I'm glad that the ProjectService API change was caught, that could have been a painful bug in the future.

I've made some comments, mostly on HTML and CSS style. Since we are now using bootstrap rules for styling, it is important that we abide by bootstrap standards. There is also a whole section that can be deleted - it was useful before we learned how to create PDF reports.

There is one test to add based on a bug that I found in the $window.confirm() alert box. That bug should be fixed before this PR can be merged.

Overall, this code and PR is looking very good. Once it is bug free and up to standards, it will be a valuable contribution.

[lomamech]

@jniles did you analyze the latest changes

[jniles]

You must let me know when to analyze changes again by leaving a comment. Otherwise, I do not know if you will add more or not.

You have one more change to make, then this can be merged in, assuming it passes tests. There's a lot of unused HTML that should be deleted.

[lomamech]

@jniles I just did it

[jniles]

LGTM.