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

chore: add API for edit user profile (#4668) #4721

Merged
merged 1 commit into from Apr 4, 2018

Conversation

tofumatt
Copy link
Contributor

@tofumatt tofumatt commented Apr 3, 2018

Gonna do the API/reducer/saga stuff for this all separately and maybe it'll make the PRs nice and easy.

Here's the API and tests for editing a user profile. I've exclude the image upload for now as I'll do that last.

Part of #4668.

@tofumatt tofumatt force-pushed the feat/edit-user-profile-reducers-4668 branch from 05c1ee0 to c9c0f1d Compare April 3, 2018 18:33
@tofumatt tofumatt changed the title WIP: chore: add API for edit user profile (#4668) chore: add API for edit user profile (#4668) Apr 3, 2018
Copy link
Contributor

@bobsilverberg bobsilverberg left a comment

Choose a reason for hiding this comment

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

Looks good,nice work!

Just a nitty comment about the tests, which you can take or leave.

expect(() => {
currentUserAccount({});
}).toThrowError(/api state is required/);
const getParams = (params = {}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a nit; take it or leave it. Considering that there's only one test that makes use of it, this getParams function is kind of unnecessary. It does seem like a good model to follow, and I know we use it in other test files. I notice the same model is followed for the other tests in this file, which also don't really need it. If more tests are added later it would be useful, but at this point none of these cases seems to need more tests.

Copy link
Contributor Author

@tofumatt tofumatt Apr 4, 2018

Choose a reason for hiding this comment

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

I think it's just a best practice we have tried to follow with test params. I might add more tests later that check if optional props are omitted, so I think it's worth keeping. It's a good point about the other tests but if we don't do this we often end up with messy tests, so I think I'm just in the habit of future-proofing them now. It makes anything that copies these tests nicer.

@tofumatt tofumatt merged commit 644413d into master Apr 4, 2018
@tofumatt tofumatt deleted the feat/edit-user-profile-reducers-4668 branch April 4, 2018 18:26
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.

None yet

2 participants