Added initial get groups tests #6531

TheHollidayInn · 2016-01-16T04:15:16Z

These are the initial get groups tests for #6509

KristianTashkov · 2016-01-16T10:41:51Z

test/api/v3/integration/groups/GET-groups.test.js

+    let leader = await generateUser({ balance: 10 });
+    user = await generateUser({balance: 4});
+
+    let publicGuildUserIsMemberOf = await generateGroup(leader, {


Is there a reason why you are not using createAndPopulateGroup for creating a group with members in it?

Probably because it's not part of the interface to add an existing user in the createAndPopulateGroup helper. Should probably add that functionality.

paglias · 2016-01-17T17:57:22Z

website/src/controllers/api-v3/groups.js

@@ -123,6 +123,10 @@ api.getGroups = {
      return previousValue.concat(Array.isArray(currentValue) ? currentValue : [currentValue]); // otherwise concat the new results to the previousValue
    }, []);

+    results = _.uniq(results, (group) => {


@TheHollidayInn why uniq? When it can result in the same challenge returned twice?

Never mind about this. For some reason the await-to-eventually-have-lengthOf function was not counting the return object correctly. I am about to push a revision.

TheHollidayInn · 2016-01-17T19:33:23Z

test/api/v3/integration/groups/GET-groups.test.js

+    let groups = await user.get('/groups?type=privateGuilds,publicGuilds,party');
+
+    await expect(groups.length)
+      .to.eql(NUMBER_OF_GROUPS_USER_CAN_VIEW);


When I put the await inside of the expect and called to.eventually.be.lengthOf this was counting incorrectly.

Why are you awaiting at all? groups.length does not return a promise.

Eh, that is left over syntax. Now, I am in the habit of adding await before my expects..

paglias · 2016-01-18T10:09:39Z

@TheHollidayInn it's possible that when you fetch public guilds the tavern is returned too? Which means that if you fetch both tavern and publicGuilds the tavern could be duplicated?

Also I'm not sure if we should populate the leader for the party and the tavern but not for other guilds, it's doesn't make sense

TheHollidayInn · 2016-01-18T14:23:37Z

....dang. Yep, that is what I was doing. I went to test that out and forgot to put back the tavern string. So, then, that is why I was using _.uniq. Should we modify something else to ensure that tavern isn't duplicated or should I just leave the string as it is?

And, I'm unsure what the populate leader code does. I was going to test it on the get group by id tests. Do you want me to look into it?

paglias · 2016-01-18T14:34:02Z

@TheHollidayInn for the tavern I think the best thing is to do something like this:

When only the tavern is fetched and not other public guilds -> fetch it as a single doc
When public guilds are fetched and the tavern too, just fetch public guilds since the tavern will be included

We could use https://docs.mongodb.org/manual/reference/operator/query/ne/ to exclude the tavern id but according to MongoDB's docs it's very inefficient.

The populateLeader option should be removed, basically it populates the leader field with the leader profile name but we don't care when listing all the guilds

TheHollidayInn · 2016-01-18T14:39:34Z

Sounds good. I can make that change in the route. I was doing that with _.uniq, but I bet if I just check for the param and skip the tavern push when we see public guilds, that would be more efficient. I'll post the solution.

Ah, I see what you are saying. Yea, I will remove the populateLeader.

crookedneighbor · 2016-01-18T15:08:51Z

Ahh, yeah, the tavern + public guilds thing is exactly what is happening. (I made a note about that in the v2 tests)

crookedneighbor · 2016-01-18T15:10:56Z

Also. Here's a crazy idea. (Not for this PR) What if we made the tavern it's own kind of group? IE: type: 'tavern'?

We're already going to need to change the id from 'habitrpg' to a uuid, right? Does it make more sense to call out that it is, in fact, a different kind of group?

paglias · 2016-01-18T17:47:00Z

@crookedneighbor hmm for the uuid I made this change and we should be fine now. At this point changing the type of group would require to change a lot of code and in fact the tavern works almost as a normal public guild except that everyone is always a members

Alys · 2016-01-18T18:10:31Z

Making the Tavern a new kind of group makes sense to me. Do we want to do the same for Party Wanted and Newbies?

paglias · 2016-01-18T18:13:28Z

@Alys @crookedneighbor I think we can have an attribute to identify official groups but not a completely different type of groups :( It would really complicate the code and there's no reason to do it as long as the functionalities are the same. And also having it compatible with api-v2 would complicate things even more. I think it can be reviewed post api v3 but not now

crookedneighbor · 2016-01-18T18:23:46Z

@paglias Good points.

TheHollidayInn · 2016-01-19T03:17:39Z

Updated with the two changes @paglias requested.

paglias · 2016-01-19T11:11:37Z

website/src/controllers/api-v3/groups.js

@@ -109,7 +109,9 @@ api.getGroups = {
          }).select(groupFields).sort(sort).exec()); // TODO use lean?
          break;
        case 'tavern':
-          queries.push(Group.getGroup({user, groupId: 'habitrpg', fields: groupFields, populateLeader: true}));
+          if (types.indexOf('publicGuilds') === -1) {


@TheHollidayInn could you remove the populateLeader at line 96? thanks

and at line 113 too

Eh, sorry about that. I didn't notice it was a parameter. I was thinking it was just in the projection.

TheHollidayInn · 2016-01-19T14:22:11Z

Updated.

…test Added initial get groups tests

KristianTashkov reviewed Jan 16, 2016
View reviewed changes

TheHollidayInn force-pushed the api-v3-group-get-groups-test branch from 8cf32e6 to 4fbca93 Compare January 17, 2016 17:22

paglias reviewed Jan 17, 2016
View reviewed changes

Added initial get groups tests

b60e0a4

TheHollidayInn force-pushed the api-v3-group-get-groups-test branch from 4fbca93 to b60e0a4 Compare January 17, 2016 19:32

TheHollidayInn reviewed Jan 17, 2016
View reviewed changes

Updated party test to conform to new leader populate

c5947ca

TheHollidayInn force-pushed the api-v3-group-get-groups-test branch from 133a7c0 to c5947ca Compare January 17, 2016 20:20

SabreCat added the status: pr: with author: needs work label Jan 18, 2016

paglias reviewed Jan 19, 2016
View reviewed changes

Ensured tavern is not returned twice and removed leader population

be55176

TheHollidayInn force-pushed the api-v3-group-get-groups-test branch from 39a7855 to be55176 Compare January 19, 2016 14:13

SabreCat added status: pr: ready for review and removed status: pr: with author: needs work labels Jan 19, 2016

paglias added a commit that referenced this pull request Jan 24, 2016

Merge pull request #6531 from TheHollidayInn/api-v3-group-get-groups-…

fe170a5

…test Added initial get groups tests

paglias merged commit fe170a5 into HabitRPG:api-v3-groups Jan 24, 2016

TheHollidayInn deleted the api-v3-group-get-groups-test branch January 24, 2016 16:23

SabreCat removed the status: pr: ready for review label Jul 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added initial get groups tests #6531

Added initial get groups tests #6531

TheHollidayInn commented Jan 16, 2016

KristianTashkov Jan 16, 2016

crookedneighbor Jan 16, 2016

TheHollidayInn Jan 16, 2016

paglias Jan 17, 2016

TheHollidayInn Jan 17, 2016

TheHollidayInn Jan 17, 2016

crookedneighbor Jan 17, 2016

TheHollidayInn Jan 17, 2016

paglias commented Jan 18, 2016

TheHollidayInn commented Jan 18, 2016

paglias commented Jan 18, 2016

TheHollidayInn commented Jan 18, 2016

crookedneighbor commented Jan 18, 2016

crookedneighbor commented Jan 18, 2016

paglias commented Jan 18, 2016

Alys commented Jan 18, 2016 via email

paglias commented Jan 18, 2016

crookedneighbor commented Jan 18, 2016

TheHollidayInn commented Jan 19, 2016

paglias Jan 19, 2016

paglias Jan 19, 2016

TheHollidayInn Jan 19, 2016

TheHollidayInn commented Jan 19, 2016

Added initial get groups tests #6531

Added initial get groups tests #6531

Conversation

TheHollidayInn commented Jan 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paglias commented Jan 18, 2016

TheHollidayInn commented Jan 18, 2016

paglias commented Jan 18, 2016

TheHollidayInn commented Jan 18, 2016

crookedneighbor commented Jan 18, 2016

crookedneighbor commented Jan 18, 2016

paglias commented Jan 18, 2016

Alys commented Jan 18, 2016 via email

paglias commented Jan 18, 2016

crookedneighbor commented Jan 18, 2016

TheHollidayInn commented Jan 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheHollidayInn commented Jan 19, 2016