204 tests for rewarding badges #223
Conversation
roschaefer
force-pushed
the
204-tests-for-rewarding-badges
branch
from
02383c2
to
9703e2d
Compare
| @@ -25,6 +25,8 @@ type Mutation { | |||
| report(id: ID!, description: String): Report | |||
| disable(id: ID!): ID | |||
| enable(id: ID!): ID | |||
| reward(fromBadgeId: ID!, toUserId: ID!): ID | |||
There was a problem hiding this comment.
I'm wondering why you return ID here 🤔 . To me, it would make sense to return [Reward]. So instead of the id of the rewarded user you would get the updated set of rewards of that user. This would simplify the test, too 😉
@mattwr18 @Tirokk this is an optional change though. I like this PR and it's mergeable, all tests pass, so I'll approve. When we touch the behaviour again (e.g. when implementing frontend part) it would make sense to improve it.
There was a problem hiding this comment.
I had talked about this in the last pairing session we had together on this @roschaefer
I didn't make changes to the logic of the PR, I just took advantage of MERGE in cypher to enforce uniqueness and got the tests passing.
It makes sense to me to return the Reward as well although @Kachulio1 was saying maybe we just return a Boolean... either way I'm happy to make the change and I'm sure @Tirokk won't have an issue with it
There was a problem hiding this comment.
Reward is a single reward [Reward] is a list of rewards. This list could reflect the list after updating. This way, you don't have to re-check this list in the test..
cc @Tirokk