Introduce search only fields #483
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When performing a search, there's additional columns that can be requested that aren't normally on the record. Previously such columns were not accessible via this gem's search results. Now these columns can be accessed just as you'd access a regular field or read only field. Starting with InventoryItem and Invoice. Ultimately, these are fields of the ___SearchRowBasic, ItemSearchRowBasic and TransactionSearchRowBasic, so eventually it may be wise to extract a module encapulating those SearchRowBasic's and include that in the relevant records to avoid duplication of search only fields across multiple records (ie. Estimate, SalesOrder, Invoice, CreditMemo, etc.) that all utilize the same SearchRowBasic.
|
Sorry for the lack of responses on the other PRs. I really like this approach. Super clean. Thanks! |
|
@cgunther it would be great to add some readme documentation around this if you have time for another PR. |
|
Thanks for the quick merge! |
cgunther
added a commit
to cgunther/netsuite
that referenced
this pull request
Aug 11, 2021
Attempt to document NetSweet#483. I wasn't sure how best to convey this given it affects consuming the search results, not performing the search itself. It should also be unsurprising that you'd access these search-only fields the same way you'd access regular or read-only fields. Had we buried these search-only fields in custom fields, like NetSweet#426 first attempted, then I'd find that more surprising and warranting clearer documentation.
iloveitaly
pushed a commit
that referenced
this pull request
Aug 11, 2021
Attempt to document #483. I wasn't sure how best to convey this given it affects consuming the search results, not performing the search itself. It should also be unsurprising that you'd access these search-only fields the same way you'd access regular or read-only fields. Had we buried these search-only fields in custom fields, like #426 first attempted, then I'd find that more surprising and warranting clearer documentation.
diegopolido
pushed a commit
to penrosehill/netsuite
that referenced
this pull request
Oct 7, 2021
When performing a search, there's additional columns that can be requested that aren't normally on the record. Previously such columns were not accessible via this gem's search results. Now these columns can be accessed just as you'd access a regular field or read only field. Starting with InventoryItem and Invoice. Ultimately, these are fields of the ___SearchRowBasic, ItemSearchRowBasic and TransactionSearchRowBasic, so eventually it may be wise to extract a module encapulating those SearchRowBasic's and include that in the relevant records to avoid duplication of search only fields across multiple records (ie. Estimate, SalesOrder, Invoice, CreditMemo, etc.) that all utilize the same SearchRowBasic.
diegopolido
pushed a commit
to penrosehill/netsuite
that referenced
this pull request
Oct 7, 2021
Attempt to document NetSweet#483. I wasn't sure how best to convey this given it affects consuming the search results, not performing the search itself. It should also be unsurprising that you'd access these search-only fields the same way you'd access regular or read-only fields. Had we buried these search-only fields in custom fields, like NetSweet#426 first attempted, then I'd find that more surprising and warranting clearer documentation.
Closed
cgunther
added a commit
to cgunther/netsuite
that referenced
this pull request
Oct 20, 2021
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.
cgunther
added a commit
to cgunther/netsuite
that referenced
this pull request
Oct 20, 2021
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
pushed a commit
that referenced
this pull request
Oct 21, 2021
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When performing a search, there's additional columns that can be requested that aren't normally on the record. Previously such columns were not accessible via this gem's search results.
Now these columns can be accessed just as you'd access a regular field or read only field.
Starting with InventoryItem and Invoice.
Ultimately, these are fields of the ___SearchRowBasic (ie. ItemSearchRowBasic and TransactionSearchRowBasic), so eventually it may be wise to extract a module encapulating those SearchRowBasic's and include that in the relevant records to avoid duplication of search only fields across multiple records (ie. Estimate, SalesOrder, Invoice, CreditMemo, etc.) that all utilize the same SearchRowBasic.
This is a second attempt at #470, which itself was a second attempt at #426.