Support Attaching Files - Revised #509
Conversation
`Records#type` conflicted with the `search_only_field :type` on `Invoice`, such that when `AttachFile` called `@object.type`, you'd get the search-only field value (often `nil`), rather than the result of `Records#type`. `Records#type` was renamed to `Records#netsuite_type` to avoid such conflicts. Furthermore, I added a check to ensure that future fields and public/protected methods don't conflict. I tried to include private methods too for completeness, but since we want to check up the ancestor tree to see if a method is defined, `search_only_field :print` on `Invoice` conflicted with `Kernel#print` in the ancestor tree. For now I opted to turning a blind eye to conflicts with private methods, however the alternative would be to translate to another method name for `search_only_field :print`, similar to the `class/klass` translation.
For a record like `SalesOrder`, this is expected to return `salesOrder`, at least for the `attach_file` call, which is the only one calling `Records#netsuite_type` currently.
cgunther
force-pushed
the
attach-file-action
branch
from
5b263c9
to
3299386
Compare
There was a problem hiding this comment.
Thank you!
Thinking ahead, if we wanted to support attaching contacts, would we introduce a new action attach_contact, or would we rename attach_file to attach, leading to a breaking change/deprecation for anyone using attach_file?
I'd be in favor of using attach_contact. attach is very vauge, and having more specific names for these methods feels more descriptive than a general attach, and would allow us to add type-specific logic more easily. I think this is worth the tradeoff of drifting from the official NetSuite name for this method.
| @@ -22,6 +22,8 @@ def fields(*args) | |||
| def field(name, klass = nil) | |||
| name_sym = name.to_sym | |||
| raise "#{name} already defined on #{self.name}" if fields.include?(name_sym) | |||
| raise "#{name} conflicts with a method defined on #{self.name}" if method_defined?(name_sym) | |||
| end | ||
|
|
||
| def record_type_without_namespace | ||
| "#{self.class.to_s.split('::').last}" |
There was a problem hiding this comment.
Yes!! I've been wanting to get something like this in the codebase for some time!
|
@cgunther one thing I'd love to see in the future are some readme additions to describe how to use this functionality. |
This is a continuation of #245, addressing some snags I had using it to address a similar need (#245 (comment)). Thanks @jordan-allan for the great starting point.
The conflict between
Records#typeandInvoice.search_only_field :typeis now resolved and there's a check to ensure future conflicts between public/protected methods and fields aren't introduced. More on this is the commit message.The casing of the record type is also resolved to be lower camelcase instead of just lowercase.
Broadly I have some concerns about calling this action
attach_fileas NetSuite actually calls itattach(with a correspondingdetach) and can be used for both files and contacts as the thing being attached to another record. My immediate need was for attaching files and that aligned with @jordan-allan's work, so I stuck on that route. Thinking ahead, if we wanted to support attaching contacts, would we introduce a new actionattach_contact, or would we renameattach_filetoattach, leading to a breaking change/deprecation for anyone usingattach_file? To support contacts, I believe the only thing that would have to change is where theattachedRecord@typeis hardcoded to "file" to pull the type from the record itself (and rename the@filevariable accordingly).