Skip to content
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

#168781705 - Edit trip request #34

Merged
merged 2 commits into from
Oct 30, 2019
Merged

Conversation

ngireric123
Copy link
Collaborator

[starts #168781705]

What does this PR do?

Update trip request

Description of Task to be completed?

  • added modify controller
  • added request middleware
  • added routes

How should this be manually tested?

  • Clone the repository from here
  • Browse to the repo directory
  • navigate to Ft-update-request-168781705 branch
  • Run npm install
  • run sequelize db:migrate
  • run sequelize db:seed:all
  • run npm run dev

- To Search Requests

  • If you are logged in
    Send a PATCH request to http://localhost:port/api/v1/requests/:ID

Postman body example

{
	"typeId": 3,
	"locationId": 1,
	
	"destinations": [
		{
			"id": 1,
			"reasons": "visit andela"
		},
		{
			"id":2,
		"reasons": "visit andela nigeria office"
		}
		]
}

Any background context you want to provide?

N/A

What are the relevant pivotal tracker stories?

#168781705

Screenshots (if appropriate)

image

image

Questions:

src/routes/api/requests.js Outdated Show resolved Hide resolved
src/routes/api/requests.js Outdated Show resolved Hide resolved
@ngireric123 ngireric123 added the WIP Work In Progress label Oct 25, 2019
@ngireric123 ngireric123 changed the title ft(edit trip request): user should be able to edit his/her trip request #168781705 - Edit trip request Oct 25, 2019
@ngireric123 ngireric123 added Ready For Review and removed WIP Work In Progress labels Oct 28, 2019
@ngireric123 ngireric123 force-pushed the Ft-update-request-168781705 branch 2 times, most recently from 7553bf1 to 7dadd80 Compare October 28, 2019 20:49
@ngireric123 ngireric123 force-pushed the Ft-update-request-168781705 branch 2 times, most recently from ee4873f to fdfb06b Compare October 29, 2019 07:14
Copy link
Contributor

@alainmateso alainmateso left a comment

Choose a reason for hiding this comment

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

Good Job @ngireric123 . Kindly make your error messages descriptive to tell the user what exactly they are doing it wrong. Also, make a validation for the params id because when I pass an incorrect id format the app breaks.
Screen Shot 2019-10-29 at 13 04 04

@ngireric123 ngireric123 requested review from alainmateso and removed request for nakiwuge, Cheza-Dzabala and sabin18 October 29, 2019 11:53
Copy link
Contributor

@nignanthomas nignanthomas left a comment

Choose a reason for hiding this comment

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

Great job @ngireric123 so far. It was a long road.

However, I suggest you have more descriptive error messages.
eg: You are not the owner of this request-id,
or for destinations' id => You are not the owner of this destination id.
In short, more descriptive messages.

Kindly make sure you test your feature extensively, why has the test coverage dropped by 2.5%?

Thanks

        - added test
        - validation

        [finishes #168781705]
@@ -3,6 +3,7 @@ import models from '../database/models';
import responseError from '../utils/responseError';
import strings from '../utils/stringsUtil';


Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line

static validateRequest(req, res, next) {
const data = req.body;
let minimumItems = 1;
let maximumItems = 30;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an empty line

let maximumItems = 30;
if (data.typeId === 1 || data.typeId === 2) {
maximumItems = 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add an empty line

}
if (data.typeId === 3) {
minimumItems = 2;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a new line

async selectPending(req, res, next) {
const userId = req.user.payload.id;
const id = parseInt(req.params.id, 10);
const requester = await requests.findOne({
Copy link
Contributor

@nakiwuge nakiwuge Oct 30, 2019

Choose a reason for hiding this comment

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

I assume you are finding a request, not a requester. Create a helper function for this query.


async validateBody(req, res, next) {
const { id } = req.params;
const destination = req.body;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you using this cont anywhere? If not remove it.

}).then(destination => {
if (!destination) {
return responseError(res, 403, `Destination of id ${element.id} does not belong to this request`);

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line

Copy link
Contributor

@nakiwuge nakiwuge left a comment

Choose a reason for hiding this comment

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

@ngireric123 good job on the implementation however kindly work on the feedback below.

  • Test validateRequest.js function to increase coverage.
  • Ensure that a user can edit a return date
  • Go through the comments I have left on your pr

@nakiwuge nakiwuge merged commit bdd3e5a into develop Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants