Improve currency component

[DedrickEnc]

Summary

This PR improves the currency input component and introduces a new design to deal with component.

Tasks accomplished

In this PR, I did followings :

• Moving the currency component code from directive to component
• Adding possibility to use max value
• Introducing new design to deal with component
• Incorporate currency select component (@jniles's code) to the currency input component (cascaded components)
• Using persistence possibility to the correct place (currency select component).

This PR closes #161

[DedrickEnc]

@jniles you can give it a review

[jniles]

@DedrickEnc, this is good code. Unfortunately, it it does close all of #161. Here is what is missing:

• strict option, making the currency input not accept parts of a currency (45 FCs, 0.0034 USD, etc)
• Default text when no currency is selected
• Remove the binding to the parent form (form), and use a child form instead to do local validation.

If you have comments on those objectives, please post them on #161.

This PR improves the bhCurrencyInput by removing the $watch() on scope, which is excellent. However, it does this by tightly coupling two components, making the developer unable to alter the appearance or placement of the bhCurrencySelect component when using the bhCurrencyInput. I'm not sure this is a better solution in that respect, but it is a good start.

I'm confused why you blocked #178 and then used it's code. You did not give any feedback on how to improve #178, and did not fix the problem from #178. The problem with #178 is that it uses $scope.$watch() implement the functionality of disable-ids. See here. Otherwise, #178 handled changed exactly the same way you do here - with an on-change() callback.

My question from https://github.com/IMA-WorldHealth/bhima-2.X/pull/178/files#r55346517 still remains. Can you produce a better design for handling the disable-ids case?

[DedrickEnc]

• A component can use an other component, there is no problem. So a currency input component is using a currency select component which remains an independent component because it can be used with out currency input component.
• The placement of currency select component in the currency input component is natural, I can not find a better position than that, otherwise I must put it at the bottom of currency input component which is strange.
• A currency select component should let user select a currency (so simple), so my question is what is the purpose of disable-ids? I tried to see the code, but you did not provide many comment about that, so it is complicate for me to understand, what is his purpose exactly?

[jniles]

I have no problem with this intercomponent communication. If every bhCurrencyInput needs to have a selection of currencies, than it makes sense to join them together into a single component. I was under the impression that there might be some places we would not have the option of choosing a currency, but would still want the power of the bhCurrencyInput's validation.

To answer your last question, some cashboxes do not have currency accounts configured for them. In this case, we should disable the currency option so that the user cannot create a transaction involving that cashbox in a different currency.

... if you didn't understand the code, why not comment asking for an explanation instead of simply removing the feature?

[jniles]

To show you what I mean about the placement of components, here is a comparison of the cash page before and after:
Cash Before

Cash After

It's not a huge problem, but in the first case, changing the currency will affect everything below the currency selection. In the second case, the currency will change both the totals above and the input below. It's a tiny regression from a usability perspective.

[DedrickEnc]

@jniles
I completed the other improvement tasks, here are things you have to know :

• I found an other alternative to perform disabled currency, so now it is handled by the currency input component not as a disabled ids but has an unsupported currency ids so the currency input component does not allow user to insert an amount if an unsupported currency is chosen and show a message.
• I remove some methods in cash which was related to date select as we should use a date component
• Now the currency input component can handle a part of currency and show a message, since angular does not recognize step attribute I used a custom validation.

[jniles]

I'll take a look at this this evening. I'd also be interested in hearing opinions from other developers. @IMA-WorldHealth/local-contributors what do you think?

[jniles]

This is a difficult design for me to support. Here are some of the issues that concern me:

1. Tightly Coupling Components: There is no way to use the bhCurrencyInput on its own without the bhCurrencySelect embedded in it. It is not difficult to think of examples where you may need to use a pre-selected currency without giving the user an option to change currency (paying a bill in USD, but still want to have the validation of the bhCurrencyInput). Additionally, this proposal removes the possibility of having multiple bhCurrencyInputs on a page. For example, in a grid. Each one will embed its own currency select.

2. No Layout Flexibility: A significant downside to this layout is that the radio buttons will always be right on top of the currency input, where I would naturally expect to find a total field or something similar. This is shown in the above images. In bhima-2.X, we are focusing on making things clean and tested, both in the code and the user experience. This proposal takes away the programmers ability do define what form components go where on their forms, and creates a strange UX for our clients.

3. Regression in UX: If a user is not allowed to pay in a currency, it makes more sense to remove their ability to choose that currency than it does to allow them to make an incorrect choice and then send a big error message at them.

4. Unclear Component Responsibility: The goal of these components is generally to perform a single function. Before, the functions where:

   1. bhCurrencySelect - take in a list of currencies and a filtering array, allowing the user to choose all currencies satisfying the filter conditions.
   2. bhCurrencyInput - ensure that a number entered into the input is an actual currency.

   This proposal adds to these responsibilities, so that bhCurrencyInput does a lot of work to validate currencies, not only based on whether the currency is valid or not, but if the module is allowed to use that currency.

For these reasons above, I am uncomfortable with this proposal. It seems to me this was a solution looking for a problem, rather than a solution to a problem. It is unclear to me what problem this code is trying to solve that is not already solved in a clearer fashion in this PR. Since there was no feedback on why the PR was not merged it is difficult to understand why this is a better proposal.

I'll wait for @sfount's opinion before merging this code.

@DedrickEnc, if you would like to present an analysis of the two proposals with reasons why this one is better, I would be interested to read it.

[DedrickEnc]

@jniles
I. I disagree, go to see the angular documentation, embedded components are not bad thing.
Using the bhCurrencyInput in the grid is possible and there are two possibilities :

• Say the currency input to not include the currency select (Recommended)
• Create an other component

II. Yesterday, about the layout problem you said it was acceptable, today it is changed?

III. The bhCurrencyInput is closed to the page than bhCurrencySelect, it makes more sense to charge the bhCurrencyInput checking this situation.
Also a choice him self can not be invalid, the choice is invalid for an other component or a page since I don't need to handle this situation in the page level, that why I prefer to do it in the currency input.
On a page a currency can only be invalid in a particular context and the context I choose for this page is currency input component, this component will consider some currency as unsupported.

IV. I disagree, where did you take this theory? a component must have clears responsibilities and that is the case for this PR :

• bhCurrencySelect just propose currencies choices to the user and store selected currency to remember (so simple)
• bhCurrencyInput receive an amount just for a supported currency. the page will inform the bhCurrencyInput about the list of unsupported currencies.

Yesterday, this PR was very good and I was supposed to add one or two more functionnalities and today every thing is bad, here is your comment :

@DedrickEnc, this is good code. Unfortunately, it it does close all of #161. Here is what is missing:

• strict option, making the currency input not accept parts of a currency (45 FCs, 0.0034 USD, etc)
• Default text when no currency is selected
• Remove the binding to the parent form (form), and use a child form instead to do local validation.

If you have comments on those objectives, please post them on #161.

This PR improves the bhCurrencyInput by removing the $watch() on scope, which is excellent. However, it does this by tightly coupling two components, making the developer unable to alter the appearance or placement of the bhCurrencySelect component when using the bhCurrencyInput. I'm not sure this is a better solution in that respect, but it is a good start.

I'm confused why you blocked #178 and then used it's code. You did not give any feedback on how to improve #178, and did not fix the problem from #178. The problem with #178 is that it uses $scope.$watch() implement the functionality of disable-ids. See here. Otherwise, #178 handled changed exactly the same way you do here - with an on-change() callback.

My question from https://github.com/IMA-WorldHealth/bhima-2.X/pull/178/files#r55346517 still remains. Can you produce a better design for handling the disable-ids case?

This PR request is a good thing, If you are not at easy to discuss about that, I will be ok to wait for @sfount even until april if possible.
Thanks

[jniles]

@DedrickEnc, I never said embedded components was a bad thing. Where did I say that? In this case, it is inappropriate. By default we embed an entire extra component, and you recommend we pass a flag to turn it off? That is poor design.

I am comparing the two designs, one proposed in #178 and this one. The design proposed in #178 is modular, composable, and decoupled. It is easy for developers to get the best user experience using it. It satisfies all the requirements without imposing any new requirements of its own. It certainly doesn't ask developers to create a new component not have the currency select radios embedded in this component.

I took the theory because I built the original components. Those were my design requirements. It's called the Single Responsibility Principle in programming. I just applied it to my components.

The code was good. Now, it is less good (for example, the validation should be done with an ngModel $validator), but still acceptable for a first PR. We could clean it up later. The design is bad. All my critiques are about the design.

[jniles]

You also never responded to the last part of my comments last week, which I hoped would help me understand why this PR even exists in the first place. Since I do not have any arguments why this is a better idea than the previous code and I've supplied plenty of critiques of this, particularly from the UI/UX perspective, I have a hard time evaluating the benefits of this PR.

[DedrickEnc]

@sfount

When ready you can review this pull request, thanks!

[DedrickEnc]

@jniles @sfount
This PR should be reviewed.
Thanks

[jniles]

@sfount, this one's on you.

[sfount]

The code changes and discussion here were factored into #414 (currently in use in many modules). This pull request contains good code that should be referenced to complete #161, however it is 7 months out of date and cannot be merged with the current master.