Skip to content
This repository has been archived by the owner on Sep 1, 2021. It is now read-only.

#46: Remove API tokens from Git (closes #46) #48

Merged
merged 4 commits into from
Feb 8, 2017

Conversation

dhinus
Copy link
Contributor

@dhinus dhinus commented Feb 7, 2017

Issue #46

Test Plan

tests performed

After setting the new env vars:

export CONTACTHUB_TEST_TOKEN="..."
export CONTACTHUB_TEST_WORKSPACE="..."
export CONTACTHUB_TEST_NODE="..."

npm run e2e

tests not performed (domain coverage)

The ContactHub API token, workspaceId and nodeId used by the e2e tests
must now be set in three env variables.

I also moved the shared `randomString()` method to the new `helper.js`
file.
Some tests were relying on specific data already being present in the
workspace.

I also had to increase the timeouts in a few places because the API is
slower than usual. Hopefully this can be reverted if the API performance
improves.
Copy link
Contributor

@ascariandrea ascariandrea left a comment

Choose a reason for hiding this comment

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

Code looks good 💯
Just a few comments left

return c.base && c.base.contacts && c.base.contacts.email;
});

expect(first && second && first > second).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you checking first > second?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To test they are reverse-sorted by email address, e.g. I expect zzzz@example.com to appear first than xxxx@example.com

README.md Outdated
```sh
export CONTACTHUB_TEST_TOKEN="..."
export CONTACTHUB_TEST_WORKSPACE="..."
export CONTACTHUB_TEST_NODE="..."
Copy link
Contributor

Choose a reason for hiding this comment

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

CONTACTHUB_TEST_WORKSPACE_ID and CONTACTHUB_TEST_NODE_ID or even better CH_TEST_WORKSPACE_ID and CH_TEST_NODE_ID :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it but was kind of annoyed by having an additional _ :)
It's probably better, anyway, I'll add it. The short version is a bit more obscure if you look at your vars in env and don't remember what CH was about, I'll use the long version.


const tok = process.env.CONTACTHUB_TEST_TOKEN;
const wid = process.env.CONTACTHUB_TEST_WORKSPACE;
const nid = process.env.CONTACTHUB_TEST_NODE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

}

return new ContactHub({
token: tok,
Copy link
Contributor

Choose a reason for hiding this comment

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

If you name tok var to token you can use the short form here (also for workspaceId, nodeId)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I'll change it.

@ascariandrea ascariandrea merged commit c6a91dc into master Feb 8, 2017
@nemobot
Copy link

nemobot commented Feb 8, 2017

@ascariandrea ascariandrea deleted the 46-remove_api_tokens_from branch February 8, 2017 11:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants