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

Added get group challenges route and initial tests #6590

Merged

Conversation

TheHollidayInn
Copy link
Contributor

This address Get challenges for group and tests in #6509. I didn't see much logic here that needed to be abstracted, @paglias . Let me know if you disagree.

@crookedneighbor
Copy link
Contributor

The _.findIndex syntax is a little clunky in the expectation. There's got to be a better way to write that expectation.

@TheHollidayInn
Copy link
Contributor Author

Yea, I agree. I was trying to use the chai .include, but the object aren't equal. I'll look through the documentation a bit. Maybe, we can extend chai to use lodash.

@crookedneighbor
Copy link
Contributor

The chai things plugin might be helpful. http://chaijs.com/plugins/chai-things

@KristianTashkov
Copy link
Contributor

@blade when I suggested the same plugin you said you prefer not to have it 😢

@KristianTashkov
Copy link
Contributor

On a more constructive note, in such cases I do:

let challengeIds = challenges.map((challenge) => {
  return challenge._id;
});

expect(challengeIds).to.include(challenge._id);

@paglias
Copy link
Contributor

paglias commented Jan 27, 2016

@TheHollidayInn the PR looks good, a few things:

  1. To keep consistency with how tasks are fetched I would rename api.getChallenges to api.getUserChallenges and use an url like GET /challenges/user.
  2. I just realized both in getUserChallenges and getGroupChallenges we want to populate the leader and group data (the code is already there but commented). For populating the leader instead of hard-coding profile.name in the selected fields you can import nameFields from '../../models/user'; as we're doing when populating the group leader

@paglias
Copy link
Contributor

paglias commented Jan 27, 2016

I just noticed we don't have tests for getUserChallenge either so it may be a good occasion to add them!

@paglias
Copy link
Contributor

paglias commented Jan 27, 2016

Actually don't add population, I'm not sure if it's really needed. We'll add it later if it becomes necessary

@crookedneighbor
Copy link
Contributor

@KristianTashkov I thought you might say that. 😄

I don't recall the exact details of why you wanted to use it, but I don't think the assertions looked quite as messy as these do. In any case, I think you were right, this would be a good plugin to add.

@TheHollidayInn
Copy link
Contributor Author

So, Chai Things does not play well with Chai Keys: chaijs/chai-things#24 .

it('should return group challenges for non member', async () => {
let challenges = await nonMember.get(`/challenges/groups/${publicGuild._id}`);

expect(_.findIndex(challenges, {_id: challenge._id})).to.be.above(-1);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can't use chai-things, I'd prefer this to be something like

let foundChallenge1 = _.find(challenges, { _id: challenge._id });
expect(foundChallenge1).to.exist;

@@ -121,7 +121,8 @@ api.joinChallenge = {
let challenge = await Challenge.findOne({ _id: req.params.challengeId });
if (!challenge) throw new NotFound(res.t('challengeNotFound'));

if (!challenge.hasAccess(user)) throw new NotFound(res.t('challengeNotFound'));
let group = await Group.getGroup({user, groupId: challenge.groupId, fields: '_id type privacy', optionalMembership: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

@TheHollidayInn why you added this check? challenge.hasAccess should haveve been enough

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I undestand, you should be able to join challenges in public guilds without being a member of the guild and I guess this fixes it. But probably this logic should be put in hasAccess

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right, good catch

Copy link
Contributor

Choose a reason for hiding this comment

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

And yes, this logic should be in hasAccess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KristianTashkov Yep, this is exactly the case.

I'll move the logic. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it seems the challenge.canView callas hasAccess, so basically, we only need to check viewAccess.

@paglias
Copy link
Contributor

paglias commented Feb 2, 2016

Except for my last comment it looks good!

@TheHollidayInn
Copy link
Contributor Author

Updated!

paglias added a commit that referenced this pull request Feb 2, 2016
Added get group challenges route and initial tests
@paglias paglias merged commit 2ad5dbc into HabitRPG:api-v3-groups Feb 2, 2016
@paglias
Copy link
Contributor

paglias commented Feb 2, 2016

@TheHollidayInn thanks!

in the end i preferred to use a modified hasAccess that will work with public guilds too so it's almost the same as canView except that it doesn't return true if you're already a member of the challenge because that's handled separately by challenge.isMember and in case will return a userAlreadyInChallenge error.

@TheHollidayInn
Copy link
Contributor Author

Sounds good, thanks!

@TheHollidayInn TheHollidayInn deleted the api-v3-get-group-challenges branch April 16, 2018 17:34
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.

4 participants