Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Feature) Current exchanged value presented on invoices #904

Merged
merged 3 commits into from
Nov 27, 2016

Conversation

sfount
Copy link
Contributor

@sfount sfount commented Nov 18, 2016

This pull requests adds a value in the selected currency to patient invoices. It uses the valid exchange rate at the time of invoice.

If the enterprise currency is selected no exchange information is displayed.

Receipt currency selection (Invoice registry module)
screenshot 2016-11-18 at 12 43 02

Receipt displaying converted $:FC amount
screenshot 2016-11-18 at 12 43 27

The primary functionality is included in this pull request. Ideally to complete:

  • Wrap currency selection from patient invoice registry module in a component (handles downloading of currencies etc.)
  • Add receipt currency selection component to patient invoice module to allow receipts to be fetched in FC as they are made To be designed

Future questions

  • What is the best user experience for selecting the currency of an invoice or bill
  • Should the entire invoice be displayed as FC - as the conversion happens on the lump sum (the total cost of the invoice) this could result in very inaccurate totals without hacks to prevent this

Closes #888
Closes #902

  • Run your code through JSHint. Check out our styleguide.
  • Run integration tests.
  • Run end-to-end tests.
  • Accurately described the changes your are making in this pull request.

@jniles jniles added this to the Finance Implementation milestone Nov 18, 2016
Copy link
Collaborator

@jniles jniles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. It's a little confusing because the currency button is in a different context from the receipt. In the future, should we put this button on the receipt modal itself?

It's a step in the right direction for sure.

@@ -66,6 +82,11 @@ function InvoiceRegistryController(Invoices, bhConstants, Notify, Session, util,
vm.loading = !vm.loading;
}

Currencies.read()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have a Notify handler.

@@ -65,7 +65,7 @@
<tr>
<td>{{this.code}}</td>
<td>{{this.text}}</td>
<td class="text-right">{{currency this.transaction_price}}</td>
<td class="text-right">{{this.transaction_price}}</td>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for precision reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to address #888

<td colspan="2">{{{translate 'EXCHANGE.INVOICE_DISCLAIMER'}}}</td>
</tr>
<tr>
<td colspan="2">{{{translate 'EXCHANGE.SET_AS'}}} 1:{{exchange}} {{{translate 'EXCHANGE.ON'}}} {{dateFormat}}</td>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would look nicer if you used the currency filter on the 1 here, I think.

@jniles
Copy link
Collaborator

jniles commented Nov 22, 2016

Note: This closes #902.

This commit adds required components and client side service
infrastructure to specifiy which currency an invoice should be made to a
patient in.
This commit updates the template and the controller for adding exchange
rate values and an exchanged total to receipts.
This commit sets up the infrastructure for the receipt currency
component. This component will provide an on update callback that
updates the controller when the user has selected a new currency for
the receipt. It will also cache the user's previous choice.
@sfount
Copy link
Contributor Author

sfount commented Nov 27, 2016

@jniles this has been updated to use a component instead of polluting the registry controller/ template. 6b91c84

Copy link
Collaborator

@jniles jniles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

}
});

ReceiptCurrencyController.$inject = ['CurrencyService', 'SessionService', 'AppCache', 'Store'];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and clean component. 👍



<div class="toolbar-item">
<bh-receipt-currency
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better practice to have close component tags, instead of having self-closing tags. On Chrome, our target, it isn't a problem ... but it's bad practice.

For reference, angular/angular.js#1953, angular/angular#5563 (comment).

@jniles jniles merged commit 7386963 into Third-Culture-Software:master Nov 27, 2016
@sfount sfount deleted the feature-franc-invoices branch January 12, 2017 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants