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

Update Estimate fields/record_refs for 2021.2 #496

Merged
merged 1 commit into from
Oct 21, 2021

Conversation

cgunther
Copy link
Contributor

A couple were missing for my use case, so I updated them based on 2021.2

This effectively removes the balance and bill_is_residential fields.
Best I can tell going back to 2014.1, they were never fields. Perhaps
this was a bad copy-paste when first introducing Estimate using
another record as a starting point?

is_multi_ship_to was also removed as it's technically a field of
TransactionSearchRowBasic, not Estimate. It never could have been
set, and at best it only could have been read after a search. Ideally
it'd be reintroduced in the future as an expansion on the work in #483,
extracting a common module to represent the fields from
TransactionSearchRowBasic.

bill_address was also removed as it last appeared in 2014.1. Now it's a
field of TransactionSearchRowBasic, so it could be re-introduced
later, like above.

billing_schedule was corrected to be a record_ref.

accountingBookDetailList, partnersList, salesTeamList,
shipGroupList, and taxDetailsList are still missing as record_refs
as their corresponding classes haven't been implemented yet.

I improved the have_field matcher to optionally take a class argument
for testing the fields that are represented by special classes.

I deviated from the standard style of multiple-fields-per-line-wrapped
for a single-field-per-line style. I found the old style hard to read,
particularly when the fields fell out of alphabetical order, when
scanning to see either what fields were available, or what fields were
already supported. I'd imagine this'll make for cleaner git diffs in the
future too.

A couple were missing for my use case, so I updated them based on 2021.2

This effectively removes the `balance` and `bill_is_residential` fields.
Best I can tell going back to 2014.1, they were never fields. Perhaps
this was a bad copy-paste when first introducing `Estimate` using
another  record as a starting point?

`is_multi_ship_to` was also removed as it's technically a field of
`TransactionSearchRowBasic`, not `Estimate`. It never could have been
set, and at best it only could have been read after a search. Ideally
it'd be reintroduced in the future as an expansion on the work in NetSweet#483,
extracting a common module to represent the fields from
`TransactionSearchRowBasic`.

`bill_address` was also removed as it last appeared in 2014.1. Now it's a
field of `TransactionSearchRowBasic`, so it could be re-introduced
later, like above.

`billing_schedule` was corrected to be a `record_ref`.

`accountingBookDetailList`, `partnersList`, `salesTeamList`,
`shipGroupList`, and `taxDetailsList` are still missing as `record_refs`
as their corresponding classes haven't been implemented yet.

I improved the `have_field` matcher to optionally take a class argument
for testing the fields that are represented by special classes.

I deviated from the standard style of multiple-fields-per-line-wrapped
for a single-field-per-line style. I found the old style hard to read,
particularly when the fields fell out of alphabetical order, when
scanning to see either what fields were available, or what fields were
already supported. I'd imagine this'll make for cleaner git diffs in the
future too.
@iloveitaly
Copy link
Member

One field per line is a great idea.

Thanks for the clear explanation about the motivation behind this change.

Keep these coming!

@iloveitaly iloveitaly merged commit 7264a51 into NetSweet:master Oct 21, 2021
@cgunther cgunther deleted the estimate-fields branch October 21, 2021 14:02
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