Skip to content

Conversation

@jordan-allan
Copy link
Contributor

  • Adds support for attaching files
  • Action added to Invoice

end

def record_type_without_namespace
"#{self.class.to_s.split('::').last}"
Copy link
Member

Choose a reason for hiding this comment

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

👍 Yes!! I've been wanting to add this for a while. Thanks!

@iloveitaly
Copy link
Member

@jdedels Thanks for this! Great PR. There are some great changes in here, and I'd like to take a closer look before I merge it in.


def type
record_type_without_namespace.downcase
end
Copy link
Member

Choose a reason for hiding this comment

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

@jdedels I'm not too familiar with the attach_file call, but I believe types should be lower camel case, i.e. customer, salesOrder, transferOrder, etc. Do you know if attach_file uses a different type case or did you just test this with single-word objects?

Thanks again for your help here! Sorry about the delay in taking a look at this

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, it should be lower camel case. I just tested with a sales order and got the following error:

Savon::SOAPFault ((soapenv:Server.userException) org.xml.sax.SAXException: salesorder is not a legal value for {urn:types.core_2020_2.platform.webservices.netsuite.com}RecordType)

Changing the downcase call to lower_camelcase fixed the issue.

Separately, #483 broke this because it introduces a type search-only field on invoices, so when the action calls type to fill out the XML request body, it calls the search-only field, returning nil (unless your record instance happens to be the result of a search), rather than calling this helper.

Broadly I'd guess this is a problem with any instance methods defined on a record colliding with fields on that record. Maybe netsuite_type is better and less chance for collision? Then maybe record_type should be renamed to netsuite_namespaced_type for consistency and clarity?

@iloveitaly
Copy link
Member

Closing this out in favor of #509

@iloveitaly iloveitaly closed this Dec 30, 2021
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.

3 participants