Expose the RUMER data through the API #7262

mbayopanda · 2023-09-20T12:31:07Z

This PR is about the expose the route : /api/stock/rumer

This route requires as parameters

depotUuid: the depot uuid
start_date: the begining date of a given month
end_date: the end date of a given month

This API route returns data like this:

Where data configurationData contains information about stock of each inventory concerned:

And dailyConsumption contains for each days of the given period, information about stock entry, exit and balance:

Example:
GET <SERVER_ADDRESS>/api/stock/rumer?depotUuid=0918809809BA11ED9604061E16E213FC&start_date=2023-09-01&end_date=2023-09-30

The corresponding report in BHIMA is:

…o expose-api-rumer

jmcameron

I looked at the code and it looks fine. Very nice to have the key functionality moved into a dedicated function so it can be used for the REST API and the Rumer report.

I tested it with bhima_test and was able to get it to work with the following URL:

http://localhost:8080/api/stock/rumer?depotUuid=F9CAEB16168443C5A6C447DBAC1DF296&start_date=2023-09-01&end_date=2023-09-31

Here are some changes I suggest:

Remove the /api/ from the REST URL for this query
Add an integration regression test for the new URL. For an example, see test/integration/inventory/lot_schedule.js in PR 7205 .

jmcameron · 2023-09-21T09:00:23Z

server/config/routes.js

@@ -909,6 +909,9 @@ exports.configure = function configure(app) {
  app.get('/reports/stock/assign', stockReports.stockAssignReport);
  app.get('/reports/stock/satisfaction_rate_report', stockReports.satisfactionRateReport);

+  // stock api for inter-operability
+  app.get('/api/stock/rumer', stockReports.rumerApi.getData);


It is good to add a route for this, but I suggest removing the /api/ part of the URL. None of our other REST urls use it and it does not seem necessary.

@jmcameron I am okay with your proposal to remove the part /api in the URL, do you have some suggestions since the route returns only data to be consumed by tiers clients?

Will /stock/rumer work?

I would approach this from a different angle. Instead of thinking of this as a separate API, I have always thought of every BHIMA route as part of the BHIMA API, and we have been trying to follow REST conventions for the most part.

From a REST perspective, I would ask "what is the required parameter?" Since it is a depotUuid in this case, I would put that in the URL and call this depots/:uuid/rumer. That way, we can think about is as "this is the RUMER report for the depot identified by depotUuid".

@jniles the function used in the API route is also used elsewhere so we do not have the freedom to make this change on the endpoint route definition without modifying other modules.

jniles

I don't have a lot of time to review the code changes, but I've done what I can.

Overall, I really like the direction of this PR - it reuses the same functions for multiple report and I think that is excellent. I agree with @jmcameron's review that it should have some integration tests.

Great work!

jniles · 2023-09-21T13:57:54Z

server/models/migrations/next/migrate.sql

  (318, 'Job Titles Management','TREE.TITLE','',57, '/titles');

 CALL add_column_if_missing('employee', 'title_employee_id', 'TINYINT(1) NOT NULL DEFAULT 0');
-ALTER TABLE `employee` ADD CONSTRAINT `employee__title_employee` FOREIGN KEY (`title_employee_id`) REFERENCES `title_employee` (`id`);
+-- ALTER TABLE `employee` ADD CONSTRAINT `employee__title_employee` FOREIGN KEY (`title_employee_id`) REFERENCES `title_employee` (`id`);


I think this might be important later.. so we might at least want to add a TODO about trying to fix it during the next release. I know it was one of the last changes @lomamech made.

Thank you @jniles

jniles · 2023-09-21T13:58:22Z

server/models/migrations/next/migrate.sql

@@ -1,5 +1,9 @@
 /* v1.28.x */

+-- Fix the constraint fails
+DELETE ru FROM role_unit ru JOIN unit u ON u.id = ru.unit_id WHERE u.`path` = "/admin/odk-settings";


Great catch!

jniles · 2023-09-21T14:04:30Z

server/controllers/stock/functions/rumer.function.js

+  params.start_date = moment(new Date(params.start_date)).format('YYYY-MM-DD');
+  params.end_date = moment(new Date(params.end_date)).format('YYYY-MM-DD');
+
+  const startDate = parseInt(moment(params.start_date).format('DD'), 10);


This is a bit complicated to read. I propose you use moment.diff() to make it a bit simpler since you are using moment anyway. Something like:

const numDays = moment(params.end_date).diff(params.start_date, 'days'); for (let i = 0; i <= numDays; i++) { headerReport.push(i); }

Or something similar (you might have to add or subtract 1 from i).

…o expose-api-rumer

mbayopanda · 2023-10-02T06:48:03Z

@jmcameron could you review my PR? Please

jmcameron · 2023-10-02T07:33:24Z

@jmcameron could you review my PR? Please

I'm testing it now.

jmcameron

Unless you have a strong reason for allowing both depotUuid and depot_uuid, I encourage you to use only depot_uuid to avoid confusion and simplify the code.

I tested with all the tests (including E2E tests) with bhima_test and it worked fine. I also tested this API manually with a large production database and it seems to work correctly.

Other than the minor issue of the depot uuid parameter name, I think this is ready to merge.

jmcameron · 2023-10-02T07:58:37Z

server/controllers/stock/functions/rumer.function.js

+ */
+async function getData(reqQuery) {
+  const params = reqQuery;
+  params.depotUuid = params.depotUuid || params.depot_uuid;


I suggest choosing one depot UUID parameter name or the other. Since the dates are line start_date, I suggest allowing only depot_uuid. Don't forget to modify the PR description with the updated parameter name.

jmcameron · 2023-10-02T07:59:36Z

server/controllers/stock/functions/rumer.function.js

+  data.header = headerReport;
+
+  const parameterOpeningStock = {
+    depot_uuid : params.depotUuid,


Switch to params.depot_uuid

jmcameron · 2023-10-02T08:00:21Z

server/controllers/stock/functions/rumer.function.js

+  };
+
+  const parameterEndingStock = {
+    depot_uuid : params.depotUuid,


params.depot_uuid

The function is already used by the RUMER report module, so I do not want to change currently codes on the client or some existing modules

jmcameron · 2023-10-02T08:00:57Z

server/models/migrations/next/migrate.sql

@@ -21,7 +21,7 @@ CREATE TABLE `title_employee` (
  UNIQUE KEY `title_1` (`title_txt`)
 ) ENGINE=InnoDB DEFAULT CHARACTER SET = utf8mb4 DEFAULT COLLATE = utf8mb4_unicode_ci;

-INSERT INTO unit VALUES
+INSERT IGNORE INTO unit VALUES


jmcameron · 2023-10-02T08:01:24Z

test/integration/stock/rumer.js

@@ -0,0 +1,89 @@
+/* global expect, agent */


Thanks for adding this!

mbayopanda · 2023-10-04T01:27:23Z

bors r+

bors · 2023-10-04T01:33:12Z

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

ci/semaphoreci/push: Integration Tests

mbayopanda added 3 commits September 14, 2023 11:21

expose RUMER api

3bbf9f1

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

37120fc

…o expose-api-rumer

Expose RUMER data through the API

78192fc

mbayopanda requested review from jmcameron and jniles September 20, 2023 12:33

jmcameron reviewed Sep 21, 2023

View reviewed changes

jniles reviewed Sep 21, 2023

View reviewed changes

jmcameron added the Stock label Sep 26, 2023

mbayopanda added 3 commits September 28, 2023 22:35

fix route name

8878125

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

6527166

…o expose-api-rumer

integration test for /stock/rumer

c6d111b

mbayopanda force-pushed the expose-api-rumer branch from 46e4ba1 to c6d111b Compare October 2, 2023 02:22

jmcameron reviewed Oct 2, 2023

View reviewed changes

bors bot merged commit afb794f into Third-Culture-Software:master Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose the RUMER data through the API #7262

Expose the RUMER data through the API #7262

mbayopanda commented Sep 20, 2023

jmcameron left a comment

jmcameron Sep 21, 2023

mbayopanda Sep 21, 2023

jmcameron Sep 21, 2023

jniles Sep 21, 2023 •

edited

Loading

mbayopanda Oct 1, 2023

jniles left a comment

jniles Sep 21, 2023

mbayopanda Sep 25, 2023

jniles Sep 21, 2023

jniles Sep 21, 2023

mbayopanda commented Oct 2, 2023

jmcameron commented Oct 2, 2023

jmcameron left a comment

jmcameron Oct 2, 2023

jmcameron Oct 2, 2023

jmcameron Oct 2, 2023

mbayopanda Oct 4, 2023

jmcameron Oct 2, 2023

jmcameron Oct 2, 2023

mbayopanda commented Oct 4, 2023

bors bot commented Oct 4, 2023

Expose the RUMER data through the API #7262

Expose the RUMER data through the API #7262

Conversation

mbayopanda commented Sep 20, 2023

jmcameron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jniles Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jniles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbayopanda commented Oct 2, 2023

jmcameron commented Oct 2, 2023

jmcameron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbayopanda commented Oct 4, 2023

bors bot commented Oct 4, 2023

jniles Sep 21, 2023 •

edited

Loading