Feature/ticket email by pierreTklein · Pull Request #313 · hackmcgill/hackerAPI

pierreTklein · 2019-01-24T00:00:44Z

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #312

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Authentication, authorization, and success

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

…ackerAPI into feature/ticketEmail

YiFeiZhang2 · 2019-01-27T18:38:36Z

routes/api/hacker.js

+         */
+        hackerRouter.route("/email/weekOf/:id").post(
+            Middleware.Auth.ensureAuthenticated(),
+            Middleware.Auth.ensureAuthorized([Services.Hacker.findById]),


This would mean ensureAuthorized is looking for a hacker with _id equal to :id, and match their accountId with the user's id. However, this isn't ever actually needed if admins are the only ones who use this route. The :self case would check hackers, but hackers should never use this route in the first case. This creates some needless obfuscation.

Also, if admins are to use this route, I would assume that they should be able to send emails to all hackers. This may make the :self case completely irrelevant.

I suggest following the approach we did for team/:id get, where we only put /email/weekOf/:all/ in the routes, and and put in an identify function into ensureAuthorized. Alternatively, we could allow hackers to send themselves weekOf emails, and put 'postSelfSendWeekOfEmail' route into hackerRoles.

For the sake of consistency + future-proofing, I think it is worth keeping this as such. In addition, is possible that we want to have a link on the frontend which says, “resent ticket email” or something.

Sure, I'm down for the idea of hackers being able to resend the email. I just think in that situation we may as well put the route into hackerRoles. It's just that right now we have a weird in-between where we check for Hacker, but there's no link at all to Hacker except a 'todo later'.

Hmm, I mean there is as much of a link to a hacker as there is to a route like hacker checkin. Of course, when we want to allow hackers to send the email to themselves, we can do /hacker/weekOf/:self!

YiFeiZhang2 · 2019-01-27T18:41:20Z

middlewares/hacker.middleware.js

+            error: {}
+        });
+    }
+    Services.Email.sendTicketEmail(account.firstName, account.email, ticketSVG, next);


Was there a reason to use callbacks here instead of await/async? It would be nice for our services to be consistent.

The email library we use doesn’t support async / await, so back when I first created the service, I followed the library’s standards. I can try promisifying it and see what happens then.

Ah I see - unfortunate

* Bump @types/express from 4.11.1 to 4.16.1 Bumps [@types/express](https://github.com/DefinitelyTyped/DefinitelyTyped) from 4.11.1 to 4.16.1. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits) Signed-off-by: dependabot[bot] <support@dependabot.com> * Bump mongoose from 5.4.5 to 5.4.6 Bumps [mongoose](https://github.com/Automattic/mongoose) from 5.4.5 to 5.4.6. - [Release notes](https://github.com/Automattic/mongoose/releases) - [Changelog](https://github.com/Automattic/mongoose/blob/master/History.md) - [Commits](Automattic/mongoose@5.4.5...5.4.6) Signed-off-by: dependabot[bot] <support@dependabot.com> * Bump @types/mongoose from 5.3.8 to 5.3.9 Bumps [@types/mongoose](https://github.com/DefinitelyTyped/DefinitelyTyped) from 5.3.8 to 5.3.9. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits) Signed-off-by: dependabot[bot] <support@dependabot.com> * Add batch script to update hacker statuses (#308) * Add teamId to expansion of query result, and keep team member ids in … (#300) * Add teamId to expansion of query result, and keep team member ids in get team * Update test * Update comments * doc updates * Bug fix * ObjectId object -> string * accountId -> hackerId * _id -> .toString() * Bump @types/mongoose from 5.3.9 to 5.3.10 Bumps [@types/mongoose](https://github.com/DefinitelyTyped/DefinitelyTyped) from 5.3.9 to 5.3.10. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits) Signed-off-by: dependabot[bot] <support@dependabot.com> * Bump mongoose from 5.4.6 to 5.4.7 Bumps [mongoose](https://github.com/Automattic/mongoose) from 5.4.6 to 5.4.7. - [Release notes](https://github.com/Automattic/mongoose/releases) - [Changelog](https://github.com/Automattic/mongoose/blob/master/History.md) - [Commits](Automattic/mongoose@5.4.6...5.4.7) Signed-off-by: dependabot[bot] <support@dependabot.com> * Feature/ticket email (#313) * WIP * Finish route * Add docs * Add tests * Switch to member who has confirmed * Fix test * Remove line * Change URL of QR code * Bump qrcode from 1.3.2 to 1.3.3 Bumps [qrcode](https://github.com/soldair/node-qrcode) from 1.3.2 to 1.3.3. - [Release notes](https://github.com/soldair/node-qrcode/releases) - [Changelog](https://github.com/soldair/node-qrcode/blob/master/CHANGELOG.md) - [Commits](soldair/node-qrcode@v1.3.2...v1.3.3) Signed-off-by: dependabot[bot] <support@dependabot.com> * Bump jslint from 0.12.0 to 0.12.1 Bumps [jslint](https://github.com/reid/node-jslint) from 0.12.0 to 0.12.1. - [Release notes](https://github.com/reid/node-jslint/releases) - [Changelog](https://github.com/reid/node-jslint/blob/master/CHANGELOG.md) - [Commits](reid/node-jslint@v0.12.0...v0.12.1) Signed-off-by: dependabot[bot] <support@dependabot.com> * Add search to sponsor (#317) * Add search to sponsor * Give sponsors full hacker, account permissions * Bump @google-cloud/storage from 2.3.1 to 2.4.1 Bumps [@google-cloud/storage](https://github.com/googleapis/nodejs-storage) from 2.3.1 to 2.4.1. - [Release notes](https://github.com/googleapis/nodejs-storage/releases) - [Changelog](https://github.com/googleapis/nodejs-storage/blob/master/CHANGELOG.md) - [Commits](googleapis/nodejs-storage@v2.3.1...v2.4.1) Signed-off-by: dependabot[bot] <support@dependabot.com> * Update week-of email (#321) * Bump mongoose from 5.4.7 to 5.4.8 Bumps [mongoose](https://github.com/Automattic/mongoose) from 5.4.7 to 5.4.8. - [Release notes](https://github.com/Automattic/mongoose/releases) - [Changelog](https://github.com/Automattic/mongoose/blob/master/History.md) - [Commits](Automattic/mongoose@5.4.7...5.4.8) Signed-off-by: dependabot[bot] <support@dependabot.com>

pierreTklein added 5 commits January 13, 2019 09:22

WIP

fd70eb1

Finish route

387d3df

Add docs

2703a94

Merge develop

7f6f23b

Add tests

423d525

pierreTklein requested a review from YiFeiZhang2 January 24, 2019 00:00

pierreTklein added 6 commits January 23, 2019 19:10

Merge branch 'develop' into feature/ticketEmail

8ad2f35

Switch to member who has confirmed

824e6a9

Merge branch 'feature/ticketEmail' of https://github.com/hackmcgill/h…

8d4b11e

…ackerAPI into feature/ticketEmail

Fix test

458c6c8

Remove line

f770156

Change URL of QR code

9ec7ca7

YiFeiZhang2 requested changes Jan 27, 2019

View reviewed changes

YiFeiZhang2 approved these changes Jan 27, 2019

View reviewed changes

pierreTklein merged commit 0bf702b into develop Jan 28, 2019

loreina deleted the feature/ticketEmail branch July 30, 2020 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ticket email#313

Feature/ticket email#313
pierreTklein merged 11 commits intodevelopfrom
feature/ticketEmail

pierreTklein commented Jan 24, 2019 •

edited

Loading

Uh oh!

YiFeiZhang2 Jan 27, 2019

Uh oh!

pierreTklein Jan 27, 2019

Uh oh!

YiFeiZhang2 Jan 27, 2019

Uh oh!

pierreTklein Jan 27, 2019

Uh oh!

YiFeiZhang2 Jan 27, 2019

Uh oh!

pierreTklein Jan 27, 2019

Uh oh!

YiFeiZhang2 Jan 27, 2019

Uh oh!

pierreTklein Jan 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

pierreTklein commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pierreTklein commented Jan 24, 2019 •

edited

Loading