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.

Sign up for GitHub

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

Raises on INSUFFICIENT_PERMISSION error from NetSuite response #405

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

Raises on INSUFFICIENT_PERMISSION error from NetSuite response #405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c973a7
Merge pull request #1 from NetSweet/master
aom Mar 8, 2019
18bb31a
Merge remote-tracking branch 'upstream/master'
aom Aug 22, 2019
979f6d1
Merge remote-tracking branch 'upstream/master'
aom Oct 7, 2019
940278f
Raises on INSUFFICIENT_PERMISSION error from NetSuite response
aom Aug 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/netsuite/errors.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ module NetSuite
class RecordNotFound < StandardError; end
class InitializationError < StandardError; end
class ConfigurationError < StandardError; end
class PermissionError < StandardError; end

# NOTE not an exception, used as a wrapped around NetSuite SOAP error
class Error
Expand Down
27 changes: 27 additions & 0 deletions lib/netsuite/response.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ def initialize(attributes = {})
@header = attributes[:header]
@body = attributes[:body]
@errors = attributes[:errors] || []

raise_on_response_errors
end

def success!
Expand All @@ -16,5 +18,30 @@ def success!
def success?
@success
end

private

def status_detail
@body &&
@body.is_a?(Hash) &&
@body[:status] &&
@body[:status][:status_detail]
end

def response_error_code
if success?
nil
else
status_detail &&
status_detail[:code]
end
end

def raise_on_response_errors
case response_error_code
when 'INSUFFICIENT_PERMISSION'
raise NetSuite::PermissionError, status_detail[:message]
end
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 other error types do you think would be added to this switch statement?

Copy link
Author

Choose a reason for hiding this comment

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

@iloveitaly I don't know yet :) By implementing switch-statement I wanted to embrace the possibility of handling more errors that should raise.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://netsuite.custhelp.com/app/answers/detail/a_id/11613 I see a lot of error types, but I don't know which will be useful here.

Copy link
Author

Choose a reason for hiding this comment

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

It seems my user account doesn't have access to that link.

Copy link
Author

Choose a reason for hiding this comment

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

Nevermind. I had to open SuiteAnswers from NetSuite Help so it authenticated me and I was able to open the link directly.

Copy link
Author

Choose a reason for hiding this comment

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

Included list to the documentation as a comment in raise_on_response_errors method.

In my use case I've burned myself with INSUFFICIENT_PERMISSION errors often enough that I wanted it to raise instead of fail silently. I can't say whether other error codes should raise but I assume someone might have a good case to add more of them to this list in future!

Copy link
Contributor

Choose a reason for hiding this comment

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

@iloveitaly @aom This makes good sense to me. Though I would think that we wouldn't want to fail silently for any unsuccessful requests.

Would it be useful to have a "generic" ResponseError raised for all other error types and then we could add to the list of specific errors as needed?

def raise_on_response_errors
  return unless response_error_code

  case response_error_code
  when 'INSUFFICIENT_PERMISSION'
    raise NetSuite::PermissionError, status_detail[:message]
  else 
    raise NetSuite::UnsuccessfulResponseError, "#{status_detail[:code]}: #{status_detail[:message]}"
  end
end

I don't know if it would be awkward to have have a mix of specific and generic errors raised

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I realize that this PR has been open for a while. Adding the generic error could definitely be done in a second phase so it doesn't hold this one up any longer.

end
end
end
17 changes: 17 additions & 0 deletions spec/netsuite/response_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,23 @@
response = NetSuite::Response.new(:success => true)
expect(response).to be_success
end

it 'throws PermissionError when response failed to INSUFFICIENT_PERMISSION' do
expect {
NetSuite::Response.new(
:success => false,
:body => {
status: {
status_detail: {
:@type => 'ERROR',
:code => 'INSUFFICIENT_PERMISSION',
:message => 'Permission Violation: The subsidiary restrictions on your role prevent you from seeing this record.'
}
}
}
)
}.to raise_error(NetSuite::PermissionError)
end
end

describe '#body' do
Expand Down