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

Add user parameter to projects. #290

Merged
merged 5 commits into from
Jun 14, 2016
Merged

Conversation

MorganEPatch
Copy link
Contributor

@MorganEPatch MorganEPatch commented Jun 13, 2016

fixes issue #289

Changes in this PR.

  • Test the parameter
  • Implement the parameter

Testing this PR.

  1. Run npm test.
  2. Create a testing instance of TimeSync, then (after authenticating) make a request to /projects?user=tschuy.

Expected Output.

  1. The two new tests should pass.
  2. You should see a list only of the projects to which tschuy belongs.

@osuosl/devs

@MorganEPatch
Copy link
Contributor Author

On a side note, I think that we should refactor the times tests to look more like this. One list of all times, then use filter() and map() to get the expected results for a given test, instead of redefining the whole list every time. It'll make it easier to maintain and reduce unnecessary redundancy.

@pop
Copy link
Contributor

pop commented Jun 13, 2016

These tests look good. To address the side note I would not refactor the time tests unless you find yourself working on the time tests anyway.
Otherwise these tests look good to me. Tests +1.

@MorganEPatch
Copy link
Contributor Author

Agreed.

@@ -198,6 +198,47 @@ module.exports = function(expect, request, baseUrl) {
});
});

describe('GET /projects?user=:username', function() {
it('returns all times for a user', function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you mean "returns all projects for a user"?

@Kennric
Copy link
Contributor

Kennric commented Jun 14, 2016

Another note for a tests refactor - we should really replace user/project/activity names with generic names like user1, user2, project1, project2, (or valid_user_1, invalid_project, etc) rather than have gwm and tschuy all over our test data. A recursive /tschuy/user_1/ should take care of that pretty easily.

@pop
Copy link
Contributor

pop commented Jun 14, 2016

LGTM +1

@Kennric
Copy link
Contributor

Kennric commented Jun 14, 2016

+1

@MorganEPatch MorganEPatch merged commit 4ba5ea1 into develop Jun 14, 2016
@mathuin mathuin deleted the tristan/project-user-parameter branch February 21, 2017 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants