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

map and expose miscellaneous fees to CCR bills #2101

Merged
merged 4 commits into from
Oct 13, 2017
Merged

Conversation

jsugarman
Copy link
Contributor

@jsugarman jsugarman commented Oct 6, 2017

What
Add CCR miscellaneous fee equivalents to the JSON api output

Why
For injection of equivalent CCCD fees into CCR.

NOTE
CCR miscellaneous fees are not synonymous with CCCD miscellaneous fees. Some
Basic Fees and Fixed fees in CCCD are handled as miscellaneous fees in CCR.

Copy link
Contributor

@colinbruce colinbruce left a comment

Choose a reason for hiding this comment

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

A couple of small questions, but if the answers are obvious, feel free to merge.

private

def quantity
1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why deliberately set this to 1? Wouldn't the app/api force a value?

Or, does the fact that the fee exist often mean the quantity is one?

I'll keep reading... :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In CCR there is only ever one advocate fee. Ideally CCR should just assume this and the other hardcoded values and we can then do away with exposing quantity, rate and amount entirely.

will speak to @cibuanokpe about this, but this has been going exposed for a while now and am just exposing the same thing in a different way here.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -95,40 +95,56 @@
},
"number_of_cases": {
"id": "/properties/bills/items/properties/number_of_cases",
"type": "integer"
"type": "string"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because CCR needs strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a long slack chat with clem about this and we have settled on using strings for any number. BigDecimals, in particular, default to strings via the BigDecimal.to_json method. There is quite alot of chatter about this stuff e.g. rails/rails#25017 . rails used to have an override but it was removed for sensible reasons. I made integers strings too firstly for consistency (CCR can just assume strings and handled appropriately) but also was reading that the JSON datatype integer is not parsed consistently by some libraries in any event - meaning you could put a float/decimal in and might get away with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@colinbruce
Copy link
Contributor

I restarted that build after a flickering test... Other than than that good to merge 👍

@jsugarman jsugarman merged commit 35bc7c9 into master Oct 13, 2017
@cleargif cleargif deleted the expose-misc-fees branch March 19, 2018 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants