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

Adds AddList to Actions #406

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

aom
Copy link

@aom aom commented Aug 16, 2018

NetSuite supports AddList operation so this'll add it into the gem.

You could do same with UpsertList with the exception that if you're using externalId's you will possibly mutate existing records. AddList will throw duplicate error instead.

I took the liberty to update NetSuite version numbers in fixtures related to this new spec.

@@ -32,6 +32,8 @@ def action(name)
self.send(:include, NetSuite::Actions::Search::Support)
when :add
self.send(:include, NetSuite::Actions::Add::Support)
when :add
self.send(:include, NetSuite::Actions::AddList::Support)
Copy link
Member

Choose a reason for hiding this comment

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

@aom do we know that every record that supports add also supports add_list? My hunch would be this isn't the case, but I could be wrong here.

If every record does support add_list we should eliminate the double when :add and add add and add_list under the same when.

Copy link
Author

Choose a reason for hiding this comment

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

This was not intentional. I'll change it to when :add_list.

AFAIK the addList is widely supported by all record types. At least I can't find any exceptions from NetSuite help section: https://system.eu2.netsuite.com/app/help/helpcenter.nl?fid=section_N3481360.html

Copy link
Member

Choose a reason for hiding this comment

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

Ok, good to know! Awesome work on this PR. Excited to get it merged!

@iloveitaly
Copy link
Member

@aom Thanks for the contribution! Apologies for the delayed response. Left a comment on the PR, otherwise this looks awesome.

r.external_id == attr[:@external_id]
end

record.instance_variable_set('@internal_id', attr[:@internal_id])
Copy link
Member

Choose a reason for hiding this comment

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

@aom what happens here if you don't set an externalID on the record you are running through addList? record would be nil and throw an exception, right?

Are the results returned by addList just a internal and external ID, or is the whole record returned?

Also feels weird to use instance_variable_set here, but without modifying every record in the gem there isn't a great alternative right now. The only thing I'd like to explore here is if we can use the results from addList to generate a new list of records to return. This would eliminate the find usage and instance_variable_set.

What do you think? I haven't worked with addList so I could be wrong here.

Copy link
Author

Choose a reason for hiding this comment

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

@iloveitaly You might confusing AddList with UpsertList. You don't need to define externalId for AddList records because they are always added to NetSuite unless other validation errors occur. With UpsertList you'll need to define a unique externalId for new records or internalId for existing records.

Yes, unfortunately AddList returns just the reference to NetSuite Record with internalId and externalId. No luck there.

But you did point out a bug!

I'll include another test case without externalId. Because instance_variable_set will throw an exception if it is called for nil record. As I said above, the externalId is not mandatory for AddList records (I use it anyway on my queries so I didn't fall into this trap).

Copy link
Author

Choose a reason for hiding this comment

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

I added a safe guard in case add_list is being called without external_id.

@aom aom force-pushed the actions-add-list branch from a366534 to 4cd971f Compare March 8, 2019 09:49
@iloveitaly iloveitaly force-pushed the master branch 2 times, most recently from 7185ffc to df796ee Compare May 28, 2019 15:55
@aom aom force-pushed the actions-add-list branch from 4cd971f to d532112 Compare October 7, 2019 08:57
@aom
Copy link
Author

aom commented Oct 7, 2019

@iloveitaly rebased this branch as well to resolve conflicts

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