Skip to content

Fix GET /users - #79

Closed
pbochis wants to merge 1 commit into
coduno:masterfrom
pbochis:get-users
Closed

Fix GET /users#79
pbochis wants to merge 1 commit into
coduno:masterfrom
pbochis:get-users

Conversation

@pbochis

@pbochis pbochis commented Sep 16, 2015

Copy link
Copy Markdown
Contributor
  • only return users which were invited by the company the current user represents

 * only return users which were invited by the company the current user represents
@lorenzleutgeb

Copy link
Copy Markdown
Contributor

Why not have this behaviour opt-in? So that you can get the users that were invited by your company by specify some query parameter?

@lorenzleutgeb lorenzleutgeb self-assigned this Sep 16, 2015
@pbochis

pbochis commented Sep 17, 2015

Copy link
Copy Markdown
Contributor Author

That's a good point, but in this PR i modify the endpoint which fetches the data for app/candidates. And imo that should be the expected behaviour for that.

@lorenzleutgeb

Copy link
Copy Markdown
Contributor

Bear in mind that although coduno/app is the only frontend we currently have, we should not design the API specifically for that, but to be useful generally. getUsers(...) is called from controllers.User(...) which in turn is wired to /users. Limiting this to list users that were invited by your company by default is somewhat unexpected, I figure. We could for example make this /company/candidates (or even /company/{key}/candidates), which would carry much more context. I think a query parameter is needed if we stick with /users, but I'd really prefer having a more explicit endpoint for that.

@pbochis pbochis closed this Oct 12, 2015
@pbochis

pbochis commented Oct 12, 2015

Copy link
Copy Markdown
Contributor Author

#129 Implements this.

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.

2 participants