-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 quest reject route and intial tests #6640
Added quest reject route and intial tests #6640
Conversation
@@ -18,6 +18,7 @@ import * as firebase from '../../libs/api-v3/firebase'; | |||
import { sendTxn as sendTxnEmail } from '../../libs/api-v3/email'; | |||
import { encrypt } from '../../libs/api-v3/encryption'; | |||
import { quests as questScrolls } from '../../../../common/script/content'; | |||
import { mockAnalyticsService } from '../../libs/api-v3/analyticsService'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we importing a mockAnalyticsService?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comments said to use this in a non production environment. I was thinking that that this was replaced somehow in production. Maybe I just misread that.
869416c
to
06c88b1
Compare
Okay, updated! And, I fixed the merge conflict. Just waiting on Travis, as usual. |
06c88b1
to
3e1fad0
Compare
Looks good 👍 Waiting for startQuest |
* | ||
* @apiSuccess {Object} Quest Object | ||
*/ | ||
api.inviteToQuest = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This route was moved to a separate quests.js file. You should remove these lines and add your reject route to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, can do.
Aside from my two remaining comments, I think this looks good. |
3e1fad0
to
eb1e8ce
Compare
|
||
let group = await Group.getGroup({user, groupId: req.params.groupId, fields: 'type quest'}); | ||
if (!group) throw new NotFound(res.t('groupNotFound')); | ||
if (!quest) throw new NotFound(res.t('questNotFound', { key: questKey })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it already validated in the invite route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I was thinking there was some added security with this, but I don't really see that being the case. I think it is good to remove. Sound good to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Please remove it. If the party quest object is being modified with an invalid key, we have bigger problems and I'd prefer it to blow up so we can track it down.
…tests, and fixed analytics import
eb1e8ce
to
442a654
Compare
Updated! |
group.markModified('quest.members'); | ||
|
||
user.party.quest.RSVPNeeded = false; | ||
user.party.quest.key = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: here we should use Group.cleanQuestProgress?
Merging manually |
updated version at #6693 |
This is the initial quest reject code for #6509 .