Skip to content

Set up APIService and expose first public methods#3

Merged
andrewatwood merged 1 commit intomainfrom
2-setup-api-service
Oct 22, 2020
Merged

Set up APIService and expose first public methods#3
andrewatwood merged 1 commit intomainfrom
2-setup-api-service

Conversation

@andrewatwood
Copy link
Contributor

@andrewatwood andrewatwood commented Oct 16, 2020

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Feature

  • What is the current behavior? (You can also link to an open issue here)
    No public methods exposed on the SDK client instance.

  • What is the new behavior (if this is a feature change)?
    Sets up the API service to make standardized API requests, and sets up the first public methods on the client via AuthResource, which contains a method isSessionValid

  • Testing

  1. Grab the permSession and permMFA cookies, and the API key from the request body on any permanent.org/api
  2. Using the credentials you grabbed from the browser, const perm = new Permanent() and test that perm.auth.isSessionValid() resolves to true

@andrewatwood andrewatwood changed the title 2 setup api service Set up APIService and expose first public methods Oct 16, 2020
@andrewatwood andrewatwood requested a review from xmunoz October 16, 2020 14:33
@andrewatwood
Copy link
Contributor Author

andrewatwood commented Oct 16, 2020

Also open to general structure and organization feedback from @slifty and @jasonaowen as well!

@andrewatwood andrewatwood linked an issue Oct 16, 2020 that may be closed by this pull request
@xmunoz
Copy link
Contributor

xmunoz commented Oct 19, 2020

I don't see any new tests for this new public method. Can you add some?

Edit: JK, wasn't looking hard enough. I'm going to pass this review along to someone else. LGTM.

@xmunoz xmunoz requested review from jasonaowen and removed request for xmunoz October 19, 2020 21:59
@xmunoz
Copy link
Contributor

xmunoz commented Oct 19, 2020

I'd also like to see the functionality added here documented in the README before merging.

@andrewatwood
Copy link
Contributor Author

Great point! Can definitely update the README.

@jasonaowen
Copy link
Contributor

@andrewatwood some of the commits in this PR were made without a committer email address, which we should fix. The rest use your personal email address; is that what you intended? Make sure to set your email address in git, and then I'd be happy to pair on rebasing to fix these commits!

Copy link
Contributor

@jasonaowen jasonaowen left a comment

Choose a reason for hiding this comment

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

Looking good!

I'll test this tomorrow, but first some feedback just from reading the code.

@andrewatwood
Copy link
Contributor Author

@jasonaowen hmm was confused for a minute trying to figure that out, but realized those email-less were edits from the GitHub UI I made on 'main' to set up some Actions and other repo housekeeping stuff, and synced back in to this branch for the PR. Didn't realize it didn't have an email on the commits, seems bizarre for a default behavior...Still open to fixing that if you'd like!

Install axios for HTTP requests. axios is async/await compatible out of
the box and allows for easy mocking, along with axios-mock-adapter and
ts-sinon for mocking during testing.

Sets up the API service to make standardized API requests, and sets up
the first public methods on the client via AuthResource, which contains
a method isSessionValid

Issue #2 - Method to validate session
@jasonaowen
Copy link
Contributor

Thanks for pairing with me on rebasing, @andrewatwood! I'll work on testing this now.

Copy link
Contributor

@jasonaowen jasonaowen left a comment

Choose a reason for hiding this comment

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

Awesome work, @andrewatwood ! This looks good to me.

I tested it by building, then running in the REPL:

$ npm run build
$ nodejs
> // omitted: assign secrets to variables
> const permsdk = require('./build/main/');
> const perm = new permsdk.Permanent({sessionToken: permSession, mfaToken: permMFA, archiveId: 0, apiKey});
> perm.auth.isSessionValid().then((result) => console.log('Is session valid? ' + result));
Promise { <pending> }
Is session valid? true

There's a small UX issue below. Your call on if you want to add it to this PR, follow up in a separate PR, or not do it at all!

@andrewatwood andrewatwood merged commit a9c2dc2 into main Oct 22, 2020
@andrewatwood andrewatwood deleted the 2-setup-api-service branch October 22, 2020 02:52
@andrewatwood andrewatwood restored the 2-setup-api-service branch October 22, 2020 02:52
@andrewatwood andrewatwood deleted the 2-setup-api-service branch October 22, 2020 02:52
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.

Method to validate session

4 participants