Bhima data collector

[lomamech]

• Implente Client side for Data Collector
• Implete API for Data Collector Management
• Extend service color js adding new color
• Implement integration test for dataCollectorManagement
• Implement E2E test for dataCollectorManagement

This module makes it possible to configure the different types of the
choice lists, the lists of choices can be in the form of trees.

• Implement client side of choice list management
• Implement API for choice list Management
• Add integration and E2E for choice List Management

This module allows to add the elements of the form of a data collector,
the elements of a form have a type, some elements can have filter
compared to other elements

• Add new component for Data Collector

• Add new component for select Survey Form Type Select

• Add new component for Select Survey List Select

• Implement client side for Survey Form management

• Implement API for survey form manangement

• Add Integration and End to End test for Survey Form management

• Implementation of APIs related to the filling of forms as well as the
  display of survey data,

• Implementation of links between patient-related forms on the patient
  management page,

• Implementing the data collection report with a small search engine

• Implementation of components for Choicelist Multipke Select, Patient
  Medical Sheet, Survey Form Select,

• Adding Integration and End to End test for Displaying Metadata and
  Filling of survey Form

closes #3863

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

Expected newline before ')' function-paren-newline

[hound]

'GridRow' is assigned a value but never used no-unused-vars

[hound]

eval can be harmful no-eval

[hound]

'data' is defined but never used no-unused-vars

[hound]

Missing space after key 'id' key-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

A space is required before '}' object-curly-spacing

[hound]

Missing trailing comma comma-dangle

[hound]

A space is required after '{' object-curly-spacing
A space is required before '}' object-curly-spacing

[lomamech]

@jniles @mbayopanda @jeremielodi All the tests pass for this PR, do not hesitate to consult me for the review of this PR

[jeremielodi]

The data collector module works perfectly, there are just few things that we have to fix

• We can put the module "Collecte des données" and "Kit de collecte de données" together for better
  understanding of users, we can just use "Collecte des données" as the unique module. So inside we
  will have the setup and filling of surveys.

• /#!/survey_form
  the add field button should be disabled if the survey is not selected

• #!/data_collector_management
  There should be a link between a survey on this grid with his configuration in /#!/survey_form page

• #!/display_metadata
  Add the Export data into Ms Excel feature
  Add the number of submissions for each servey

[jniles]

@lomamech,

This PR is incredible - it represents a lot of work, and it performs very well. Well done!

As discussed, I've limited my review to the DisplayMetaData modules for the moment, and mostly the graphical user interface.

Let me know if you have any questions!

[jniles]

I'm not a fan of this padding - it looks a bit off. There is a bootstrap class that is pretty close to what you want though - it is called .well. See how it works below.

Would you be willing to change these styles to class="well"?

[jniles]

Let's comment out the "separator" until the rest are done. It makes the dropdown menu look a bit funny:

[jniles]

Instead of having this large DisplayMetadataCtrl.download() call, it would be better to move this into the controller in a function called downloadPDF(). This href will become ng-href="/data_key/report?{{DisplayMetadataCtrl.downloadPDF()".

And then in your controller you can make a function:

ctrl.downloadPDF = () => {
  return ctrl.download('pdf', ctrl.changes, ctrl.collectorId, /* etc ... */ );
};

[jniles]

You have two ng-if="DisplayMetadataCtrl.patientData" next to each other. Can you merge them?

[jniles]

Why do you specify patient and patientUuid as two different parameters?

[lomamech]

The parameter 'patient' contains patient information for the purpose of no longer querying the database for its information

[jniles]

Can you just use patient.uuid?

[jniles]

We already checked for this in the startup code with hasPatientData. Could we use that variable instead of $state?

if (hasPatientData) {
  // do x
} else {
  // do y
}

[jniles]

Let's make this into a boolean:

const hasNoChanges = Object.keys(vm.changes).length === 0;

Then we can use it in a nice check:

if (cache.changes && hasNoChanges) {
  // do x
}

[jniles]

Does vm.params depend on the result of SurveyForm.read()? If not, let's move it out:

vm.params = {
  data_collector_management_id : vm.collectorId,
  changes : vm.changes,
};

SurveyForm.read(/* etc ...*/)

[jniles]

You have a nice promise chain here that sends a request for:

1. SurveyForm.read()
2. DisplayMetadata.read()
3. DataCollectorManagement.read()

However, it doesn't look like any of these actually depend on one another. So, let's send them in parallel using Promise.all()!

vm.params = {/* etc  */};
return Promise.all([
  SurveyForm.read(null, { data_collector_management_id : vm.collectorId }),
  DisplayMetadata.read(null, vm.params),
  DataCollectorManagement.read(vm.collectorId),
])
.then(([survey, data, collector]) => {
  // process all queries...
});

[lomamech]

@jniles, i try this options but the displaying became too slow

[jniles]

See if you can do a similar Promise.all() system here.

[lomamech]

The displaying became very very too slow

[kwilu]

Hello @lomamech, it seems like you have referenced #3863 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #3863..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: kwilu
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

Thank you for your contributions to BHIMA!

[hound]

'filters' is never reassigned. Use 'const' instead prefer-const

[kwilu]

Hello @lomamech, it seems like you have referenced #3863 in your pull request description, but you have not referenced them in your commit message description(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #3863..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: kwilu
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

Thank you for your contributions to BHIMA!

[lomamech]

@jniles I have finish with the first review

[jniles]

@lomamech Great work on making these changes.

I'm still working through most of the code, but I've made a few more comments. Take a look :)

👍

[jniles]

It looks like the parentId is optional (line 36). This should be <?.

[jniles]

This should also be optional <?.

[jniles]

Most of our $state changes use $state.go(). Can you use that there?

[jniles]

Why does this need to be wrapped in a <a> tag? Can you just put it in plain <span> tag instead?

[jniles]

$state.go() should be sued here as well.

[jniles]

This shouldn't be AccountReferenceModal.

[jniles]

This probably shouldn't be AccountReferenceModal.

[jniles]

Instead of !surveyForm.$dirty, go ahead and use surveyForm.$pristine. It is the same thing, but without a !.

[jniles]

Can you use $state.go() here?

[jniles]

I propose we name this something else - "medical sheet" makes it sound like it has something to do with the patient's medical information, but it is actually just data collections about the patient.

Let's call this <bh-patient-data-collection-forms> or something like that.

[lomamech]

the use of the term "medical sheet" simply wanted to mention that this area is dedicated to medical information of patients.

Logically, the forms that should appear on this zone should only be those collecting medical information

[jeremielodi]

I propose that we use "Survey Form management" instead of "Survey Form" and
"Survey Form setting" instead of "Data Collector Management", that way the module names will be more understandable

[hound]

Line 5 exceeds the maximum line length of 120 max-len

[jeremielodi]

Make sure the user can see a warning message if he submit an invalid form.
Currently you return null in your JavaScript code for this problem.

[lomamech]

@jniles @jeremielodi I wait another review

[jniles]

@lomamech, a few more comments. Your changes from last time look good.

[jniles]

Hmm... looks like we have identical code in the AccountService. Is there any way we can move this into a shared service so that we don't have to duplicate code?

[jniles]

This translate tag needs to be on the <strong> not the <span>.

[jniles]

Toggle loading indicator always sets "loading" to be "false". Shouldn't it turn loading on and off?

[jniles]

Just FYI - for functions, you don't need to put them in the $onInit call. The $onInit() is most for variables that are passed in and/or need to be initialized with each component.

[jniles]

What is type === '4'? Why not type === '7'?

[lomamech]

It's form multiple value, look in bhima.sql in table survey_form_type

[jniles]

Can you instead use the label? It will make it much easier for a developer to read. Like this:

f.label === 'FORM.LABELS.SELECT_MULTIPLE' // now all future developers knows what this means!

[jniles]

As discussed together, I think that you can simply do FillFormForm[formItem.name].$invalid and save yourself a bit of parsing.

[jniles]

Do you need this <div> here?

[jniles]

Why not use ngRequired?

[jniles]

Since you are doing a lot of formItem.typeForm === "x", could you use an ngSwitch? It would probably make the code easier to read.

[jniles]

So .. it looks like you are taking a list of ids and turning them into a list of labels. So, you are trying to do:

var ids = [3, 3, 5, 7, 1]
// do something
console.log(ids) // => ['formA', 'formA', 'formZ', 'formX', 'formT']

Let's try to clean this up a bit. Here is what I think the basic structure could look like:

angular.forEach(vm.reportDetails.mulitpleChoice, (values, key) => {
  // values is an array.
  values.forEach(value => {
    vm.choicesLists.forEach(list => {
      if (list.id === value) {
         // replace value with list.label
      }
    });
  });
});

Let's see if we can go one step further:

// create a way to map id -> label for the choice list
let choiceListMap = vm.choicesList.reduce((map, item) => {
  map[item.id] = item.label;
}, {});

angular.forEach(vm.reportDetails.mulitpleChoice, (values, key) => {
  // unfortunately, angular.map() doesn't exist for object.  We have to use an ugly forEach
  vm.multipleChoice[key] = values.map(value => choiceListMap[value]);
});

I think that should do it.

First, we create an object to make choiceList ids map to their labels. This takes up memory but removes the choiceLists.forEach() loop.

Then, we use angular.forEach() to loop through the object. This removes all the Object.keys() calls and checks for .length.

Try it out and see if I've missed anything.

[kwilu]

Heads up @lomamech, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

[hound]

Unnecessary escape character: " no-useless-escape

[hound]

'expect' is assigned a value but never used no-unused-vars

[hound]

Unnecessary escape character: " no-useless-escape

[hound]

Unnecessary escape character: - no-useless-escape

[hound]

Line 9 exceeds the maximum line length of 120 max-len

[hound]

'FormatTreeData' is defined but never used no-unused-vars

[hound]

'Api' is defined but never used no-unused-vars

[lomamech]

@jniles @mbayopanda @jeremielodi can I have another Review

[jniles]

bors r+

[bors]

Build failed

• continuous-integration/travis-ci/push

[lomamech]

@jniles @mbayopanda After several attempts this old PR is ready to be merged

[jniles]

I'm hoping we can just get this in. Let's do it bors.

bors r+

[bors]

Build failed

• continuous-integration/travis-ci/push

[jniles]

bors try

[bors]

try

Build failed

• continuous-integration/travis-ci/push

[jeremielodi]

bors r+

[bors]

Build succeeded

• continuous-integration/appveyor/branch
• continuous-integration/travis-ci/push