FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Create travel api by logan-r · Pull Request #639 · hackmcgill/hackerAPI · GitHub

/ hackerAPI Public

Create travel api - #639

Merged
logan-r merged 15 commits into
developfrom
create-travel-api
Jan 14, 2020
ConversationCommits (15)ChecksFiles changed
Merged

Create travel api#639
logan-r merged 15 commits into
developfrom
create-travel-api

Conversation

logan-r commented Jan 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Tickets:

  • HCK-

List of changes:

Create a travel model and api. Model is of form

{
  _id: ObjectId // The id of the travel document
  hackerId: ObjectId // The id of the hacker that is traveling
  accountId: ObjectId // The id of the user that is traveling
  request: number // The amount of money the hacker is requesting for travel
  offer: number // The amount of money we are giving the hacker for travel
  status: string // The state of the hacker's travel request
}

Following api end points were also created:

  • GET /travel/:id gets a travel with a certain id
  • GET /travel/self gets travel associated with the logged in user
  • GET /travel/email/:email gets the travel associated with the user of a given email
  • POST /travel creates a new travel
  • PATCH /travel/status/:id updates the status field of a given traveler
  • PATCH /travel/offer/:id updates the amount of money we are offering a given traveler

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How has this been tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Questions for code reviewers?

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
  • My changes generate no new warnings
  • Listed change(s) in the Changelog
  • 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
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged and published in downstream modules

logan-r requested a review from loreina January 14, 2020 15:12
logan-r mentioned this pull request Jan 14, 2020
Merged
7 tasks
printharsh reviewed Jan 14, 2020
View reviewed changes
Comment thread middlewares/travel.middleware.js
}
});
}
req.body.travelDetails.request = hacker.application.accommodation.travel;

printharsh Jan 14, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

return next()?

Comment thread middlewares/travel.middleware.js Outdated
* @param {JSON} res
* @param {(err?)=>void} next
* @return {void}
* @description Load travel request from hacker application and add it to

printharsh Jan 14, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: add it to req.body.travelDetails

Comment thread middlewares/travel.middleware.js
const travelDetails = req.body.travelDetails;

const exists = await Services.Hacker.findByAccountId(
travelDetails.accountId

printharsh Jan 14, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Need to change this to instead look for a travel record by accountID instead to ensure we don't make duplicate travel docs for same linked accs. The way it is now, it is just looking for a hacker associated to an accountID which is not what we want I think.

logan-r Jan 14, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

True true

logan-r requested a review from printharsh January 14, 2020 17:01
loreina approved these changes Jan 14, 2020
View reviewed changes

loreina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM! functionality seems great

@MrHarshPatel if it's good on the technical side, ready to merge

Ty-Won reviewed Jan 14, 2020
View reviewed changes
Comment thread constants/routes.constant.js
post: {
requestType: Constants.REQUEST_TYPES.POST,
uri: "/api/travel/"
},

Ty-Won Jan 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

what is this post? Did you mean create travel?

logan-r Jan 14, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, it's for create - thought I was following the naming convention from account and hacker, would it be better to rename this from post to createTravel?

Comment thread controllers/travel.controller.js
function updatedTravel(req, res) {
return res.status(200).json({
message: Constants.Success.TRAVEL_UPDATE,
data: req.body

Ty-Won Jan 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should this be json like the other portions or no?

Comment thread controllers/travel.controller.js
Error: require("../constants/error.constant")
};

function okay(req, res) {

Ty-Won Jan 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: okay sounds a bit odd as a function name

logan-r Jan 14, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Whoops, this function was just for debugging - I'll remove it

Comment thread middlewares/travel.middleware.js
function addDefaultStatusAndOffer(req, res, next) {
req.body.travelDetails.status = "None";
req.body.travelDetails.offer = 0;
return next();

Ty-Won Jan 14, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can this be added in parseTravel? or is this used for a specific purpose?

logan-r merged commit 0040e20 into develop Jan 14, 2020
logan-r deleted the create-travel-api branch January 14, 2020 22:44
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Ty-Won Ty-Won left review comments
loreina loreina approved these changes
printharsh Awaiting requested review from printharsh

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL