Moving debtor groups functions from ledger.js to debtorGroups.js #80

mbayopanda · 2016-02-05T09:23:02Z

This PR :

move debtor group functions from ledger.js to debtorGroups.js
add debtor group API routes in routes.js
move tests relatives to debtor groups from debtors.js to debtorGroups.js

jniles · 2016-02-05T09:26:16Z

server/controllers/finance/debtors.js

+function getGroupInvoices (id) {
+  var def = q.defer();
+
+  if (!id) { def.reject(new Error('ERR_MISSING_REQUIRED_PARAMETERS')); }


You should use a return here to avoid making any database queries at all.

jniles · 2016-02-08T19:28:08Z

server/config/routes.js

@@ -378,12 +379,19 @@ exports.configure = function (app) {
  app.get('/patients/search/name/:value', patient.searchFuzzy);
  app.get('/patients/search/reference/:value', patient.searchReference);

-  // Debtors API
+  /** Debtors API */
+  /** @fixme `/debtors/groups` is deprecated, use `/debtor_groups` at the client side */


For future reference, there is a @deprecated tag in JSDoc. Pretty cool, huh?

jniles · 2016-02-08T19:51:38Z

@mbayopanda, this is an excellent pull request. You can ignore all my inline comments, they are for your information, not necessary fixes that need to land before this PR is merged.

You are two critical tests for common conditions, that must be implemented before this API is usable:

GET /debtor_groups/:uuid/invoices must return an empty array and a 200 success code, even if the debtor group does not have any transactions in the posting journal.
GET /debtor_groups/unknownId/invoices must return a 404 NOT FOUND to the client. The client will interpret it as "the debtor group by this ID was not located" and inform the user appropriately.

These are two cases that will come up a lot. Once these two cases are implemented with passing tests, I'll merge this PR. It's a really good PR.

mbayopanda · 2016-02-10T08:19:51Z

@jniles i'm still waiting for your comment about this pull request, and in waiting that, i try some things with the breadcrumb directive

jniles · 2016-02-10T08:22:16Z

server/controllers/finance/debtorGroups.js

+
+  db.exec(debtorDetailsQuery, [uuid])
+  .then(function (rows) {
+    if (!rows.length) { return next(new req.codes.ERR_NOT_FOUND()); }


For future reference, since you are in a promise chain, this line of code is equivalent:

if (!rows.length) { throw new req.codes.ERR_NOT_FOUND(); }

jniles · 2016-02-10T08:36:51Z

@mbayopanda , next time ping me with @jniles as soon as you've finished your changes.

Your tests look good for the /invoices route.

I've completed a second review. I know this PR is taking a while but the GET /debtor_groups route needs work. If you could write an integration test (or two) to show that the following queries perform as expected:

GET /debtor_groups?locked=0 should not return a locked debtor group.

After this change, we'll have a lot more confidence to use the API. Since you will be working on the client-side next, I think it will be a valuable change to make now.

…into update_debtors_module

jniles · 2016-02-10T14:38:48Z

server/test/api/debtorGroups.js

+var expect = chai.expect;
+
+var helpers = require('./helpers');
+var uuid    = require('node-uuid');


It's excellent to see you using node-uuid. This is a step in the right direction!

jniles · 2016-02-10T14:43:04Z

LGTM.

Moving debtor groups functions from ledger.js to debtorGroups.js

2447: Update chai-spies to the latest version 🚀 r=jniles a=greenkeeper[bot] ## Version **1.0.0** of [chai-spies](https://github.com/chaijs/chai-spies) was just published. <table> <tr> <th align=left> Dependency </td> <td> chai-spies </td> </tr> <tr> <th align=left> Current Version </td> <td> 0.7.1 </td> </tr> <tr> <th align=left> Type </td> <td> dependency </td> </tr> </table> The version **1.0.0** is **not covered** by your **current version range**. If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update. It might be worth looking into these changes and trying to get this project onto the latest version of chai-spies. If you have a solid test suite and good coverage, a passing build is a strong indicator that you can take advantage of these changes directly by merging the proposed change into your project. If the build fails or you don’t have such unconditional trust in your tests, this branch is a great starting point for you to work on the update. --- <details> <summary>Commits</summary> <p>The new version differs by 44 commits.</p> <ul> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/1f79da565cb6bb0d05968341179e5af3be786e28"><code>1f79da5</code></a> <code>chore(package): update rollup to version 0.53.4</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/ededa6af6786b9df2fdf5dbdb85dc921394e71e1"><code>ededa6a</code></a> <code>Merge pull request #93 from stalniy/feat/fn-body</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/9b599ca4b948a869dbf9397aeda63c609f21eaad"><code>9b599ca</code></a> <code>feat(spy): adds original function body to be exposed in spy.toString()</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/9a2d248098a2dfd7e804cc3691f8d8ad786b6116"><code>9a2d248</code></a> <code>docs(README): adds documentation about sandboxes and new methods</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/55ad5a5eca809015bddf69b9793125133b618adb"><code>55ad5a5</code></a> <code>1.0.0</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/509926a72174b178f0d78182a7b7a4463068550d"><code>509926a</code></a> <code>Merge pull request #91 from stalniy/fix/updates</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/27d9769faced90475edd22bf997cd3c1a71f53b9"><code>27d9769</code></a> <code>Fix formatting if readme</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/fc83ab53395c987c4d8729b895846d72835b9f83"><code>fc83ab5</code></a> <code>chore(travis): updates list of testing node versions</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/19dfebca47f0257e3095cf79249f01f230cf8ecb"><code>19dfebc</code></a> <code>chore(deps): upgrades rollup</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/c27ab283d2ecb7b74141aa62c1b4d20f9991a603"><code>c27ab28</code></a> <code>chore(package): drop support for node 0.x</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/f8c4df7d553e5e8c57a9c0ce0296b4052b00077f"><code>f8c4df7</code></a> <code>Merge pull request #84 from damianb/patch-2</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/fe7ed2a5cdef162bb97c180a92d731aa57c75a3a"><code>fe7ed2a</code></a> <code>Remove link to very dead project</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/fa6a9b05848952859134743e9ee6dd4f07c73f38"><code>fa6a9b0</code></a> <code>Merge pull request #80 from mLuby/patch-1</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/dab43739a19feb38be8ca5d9801d38cf6bc93b68"><code>dab4373</code></a> <code>Update Chai URL in README</code></li> <li><a href="https://urls.greenkeeper.io/chaijs/chai-spies/commit/e7530f11dbd4b4bdf4ef8d52d41ac2d2f429b9e9"><code>e7530f1</code></a> <code>Merge pull request #63 from chaijs/remove-lgtm</code></li> </ul> <p>There are 44 commits in total.</p> <p>See the <a href="https://urls.greenkeeper.io/chaijs/chai-spies/compare/9309677747faae40badb5219d1ef41d039273a60...1f79da565cb6bb0d05968341179e5af3be786e28">full diff</a></p> </details> <details> <summary>FAQ and help</summary> There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new). </details> --- Your [Greenkeeper](https://greenkeeper.io) bot 🌴

jniles reviewed Feb 5, 2016
View reviewed changes

mbayopanda added 2 commits February 8, 2016 13:49

Moving debtor groups functions from ledger.js to debtors.js

1fa75ed

Debtor group API

ed0dc9c

mbayopanda changed the title ~~Moving debtor groups functions from ledger.js to debtors.js~~ Moving debtor groups functions from ledger.js to debtorGroups.js Feb 8, 2016

Use of q immediatelly instead of an q.defer() object

a1f9398

mbayopanda force-pushed the update_debtors_module branch from e1c0003 to a1f9398 Compare February 8, 2016 13:19

jniles reviewed Feb 8, 2016
View reviewed changes

Test and Handling error message in debtorGroups.js controller

b23813f

jniles reviewed Feb 10, 2016
View reviewed changes

mbayopanda added 3 commits February 10, 2016 10:33

Merge branch 'master' of https://github.com/IMA-WorldHealth/bhima-2.X …

b73faac

…into update_debtors_module

Implementation of POST /debtor_groups/ and Tests enhancement

68a3a81

Fixing debtors.js test by removing all debtor group's tests

011dce8

jniles reviewed Feb 10, 2016
View reviewed changes

jniles added a commit that referenced this pull request Feb 10, 2016

Merge pull request #80 from mbayopanda/update_debtors_module

618f688

Moving debtor groups functions from ledger.js to debtorGroups.js

jniles merged commit 618f688 into IMA-WorldHealth:master Feb 10, 2016

mbayopanda deleted the update_debtors_module branch September 14, 2016 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving debtor groups functions from ledger.js to debtorGroups.js #80

Moving debtor groups functions from ledger.js to debtorGroups.js #80

mbayopanda commented Feb 5, 2016

jniles Feb 5, 2016

jniles Feb 8, 2016

jniles commented Feb 8, 2016

mbayopanda commented Feb 10, 2016

jniles Feb 10, 2016

jniles commented Feb 10, 2016

jniles Feb 10, 2016

jniles commented Feb 10, 2016

Moving debtor groups functions from ledger.js to debtorGroups.js #80

Moving debtor groups functions from ledger.js to debtorGroups.js #80

Conversation

mbayopanda commented Feb 5, 2016

jniles Feb 5, 2016

Choose a reason for hiding this comment

jniles Feb 8, 2016

Choose a reason for hiding this comment

jniles commented Feb 8, 2016

mbayopanda commented Feb 10, 2016

jniles Feb 10, 2016

Choose a reason for hiding this comment

jniles commented Feb 10, 2016

jniles Feb 10, 2016

Choose a reason for hiding this comment

jniles commented Feb 10, 2016