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 UserDescriptor to branches.list #293

Merged
merged 10 commits into from
Jul 16, 2020

Conversation

timc1
Copy link
Contributor

@timc1 timc1 commented Jul 14, 2020

While working on Abstract Home's My Active Branches, I needed to grab all the branches from a specific user.

This PR adds a UserDescriptor as an option to the branches.list descriptor, so we can pass userId to the query params and get all branches for that user.

@timc1 timc1 requested review from amccloud and tommoor July 14, 2020 21:14
Copy link
Contributor

@amccloud amccloud left a comment

Choose a reason for hiding this comment

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

Coming together 👍

Thanks for updating test. You'll also want to update docs

src/endpoints/Branches.js Show resolved Hide resolved
src/endpoints/Branches.js Outdated Show resolved Hide resolved
@timc1 timc1 requested review from amccloud and tommoor July 15, 2020 19:31
@timc1 timc1 marked this pull request as ready for review July 15, 2020 19:31
Copy link
Contributor

@amccloud amccloud 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 👍

Make sure you update the docs and typescript interface to reflect these changes

query = querystring.stringify({
...queryOptions,
userId: descriptor.userId
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I get the intention here and it'll work until you're using types

list({ projectId, userId }) is not currently allowed so userId can't be present in the condition below. So query is only shared in the else branch but not for the projects branch

If we want to allow filtering branches for a user in a project you'll also want:

descriptor?: ProjectDescriptor | ProjectMembershipDescriptor | UserDescriptor

where type ProjectMembershipDescriptor = {| projectId: string, userId: string |}

It looks like that type exist but might be mislabeled and mixed with OrganizationProjectDescriptor which doesn't appear to be used

@timc1
Copy link
Contributor Author

timc1 commented Jul 16, 2020

@amccloud thanks for the thorough review! I went ahead and updated the type definition, docs, as well as the descriptor mismatch.

docs/abstract-api.md Outdated Show resolved Hide resolved
Co-authored-by: Tom Moor <tom@abstract.com>
let response = null;

if (descriptor && descriptor.userId) {
query = querystring.stringify({
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 fine and I'm just nit'ing but you could avoid running stringify twice and neaten this up a little by just adding userId to the queryOptions inside of this block and then run querystring.stringify afterwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ty 7c445f0

@timc1 timc1 requested review from tommoor and amccloud July 16, 2020 17:35
…m:goabstract/abstract-sdk into chore/add-user-descriptor-for-branch-list
@timc1 timc1 merged commit 0849b63 into master Jul 16, 2020
@timc1 timc1 deleted the chore/add-user-descriptor-for-branch-list branch July 16, 2020 18:07
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.

3 participants