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.

Sign up for GitHub

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

Jump to bottom

update edit page 1396 #1401

Closed
wants to merge 4 commits into from
Closed

update edit page 1396 #1401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
208f364
update edit page 1396
plang-psm Jun 20, 2023
d33c125
update to PUT request
plang-psm Jun 21, 2023
b11873a
change patch to put in test
plang-psm Jun 21, 2023
159309a
adding patch back to router due to test failing for delete
plang-psm Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions backend/routers/projects.router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const express = require("express");
const express = require('express');
Copy link
Member

Choose a reason for hiding this comment

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

Can you please drop this change as per other comments in this PR review re: linting

const router = express.Router();

const { ProjectController } = require('../controllers');
Expand All @@ -10,7 +10,8 @@ router.post('/', ProjectController.create);

router.get('/:ProjectId', ProjectController.project_by_id);

router.patch('/:ProjectId', ProjectController.update);
router.put('/:ProjectId', ProjectController.update);

router.patch('/:ProjectId', ProjectController.update);

module.exports = router;
56 changes: 22 additions & 34 deletions backend/routers/projects.router.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,37 +20,31 @@ describe('CREATE', () => {
};

// Submit a project
const res = await request
.post('/api/projects/')
.set(headers)
.send(submittedData);
const res = await request.post('/api/projects/').set(headers).send(submittedData);
expect(res.status).toBe(201);
done();
});
});

describe('READ', () => {
test('Get all projects with GET to /api/projects/', async (done) => {
// Test Data
const submittedData = {
name: 'projectName',
};

// Submit a project
const res = await request
.post('/api/projects/')
.set(headers)
.send(submittedData);
expect(res.status).toBe(201);

// Get all projects
const res2 = await request.get('/api/projects/').set(headers);
expect(res2.status).toBe(200);

const APIData = res2.body[0];
expect(APIData.name).toBe(submittedData.name);
done();
});;
// Test Data
const submittedData = {
name: 'projectName',
};

// Submit a project
const res = await request.post('/api/projects/').set(headers).send(submittedData);
expect(res.status).toBe(201);

// Get all projects
const res2 = await request.get('/api/projects/').set(headers);
expect(res2.status).toBe(200);

const APIData = res2.body[0];
expect(APIData.name).toBe(submittedData.name);
done();
});
});

describe('UPDATE', () => {
Expand All @@ -61,10 +55,7 @@ describe('UPDATE', () => {
};

// Submit a project
const res = await request
.post('/api/projects/')
.set(headers)
.send(submittedData);
const res = await request.post('/api/projects/').set(headers).send(submittedData);
expect(res.status).toBe(201);

const updatedDataPayload = {
Expand All @@ -73,7 +64,7 @@ describe('UPDATE', () => {

// Update project
const res2 = await request
.patch(`/api/projects/${res.body._id}`)
.put(`/api/projects/${res.body._id}`)
Copy link
Member

@trillium trillium Jul 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I believe you've only materially edited one line in this file. It's hard to pick out what's being worked on and what isn't if we get lots of changes from your linter. Can you please go and pick out the specific part of the code you're changing? These steps shoud do the trick:

Rebase recent changes

git rebase -i HEAD~4

Locate commit with your code

pick 208f364 update edit page 1396
pick d33c125 update to PUT request
pick b11873a change patch to put in test // This one
pick 159309a adding patch back to router due to test failing for delete

Change pick to edit on b11873a -- this brings your repo to that specific state so you can change / edit things

pick 208f364 update edit page 1396
pick d33c125 update to PUT request
edit b11873a change patch to put in test // This one
pick 159309a adding patch back to router due to test failing for delete

Save and close file

Git now brings you to that specific commit state. Reset your changes to the previous HEAD. This unstages the changes you've made in this specific commit.

git reset HEAD^

Now use git add --patch to select the specific part of your edit that is relevent to this commit:

git add --patch backend/routers/projects.router.test.js

This will bring up an interactive selection in your terminal (complete with colors that won't show in markdown)

     // Submit a project
-    const res = await request
-      .post('/api/projects/')
-      .set(headers)
-      .send(submittedData);
+    const res = await request.post('/api/projects/').set(headers).send(submittedData);
     expect(res.status).toBe(201);
     done();
   });
(1/5) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?

What do these mean? [y,n,q,a,d,j,J,g,/,e,?]

  patch
       This lets you choose one path out of a status like selection. After choosing the path, it presents the diff between the index and the working tree file and asks you if you want to stage the change of each hunk. You can select one of the following options and type return:

           y - stage this hunk
           n - do not stage this hunk
           q - quit; do not stage this hunk nor any of the remaining ones
           a - stage this hunk and all later hunks in the file
           d - do not stage this hunk nor any of the later hunks in the file
           g - select a hunk to go to
           / - search for a hunk matching the given regex
           j - leave this hunk undecided, see next undecided hunk
           J - leave this hunk undecided, see next hunk
           k - leave this hunk undecided, see previous undecided hunk
           K - leave this hunk undecided, see previous hunk
           s - split the current hunk into smaller hunks
           e - manually edit the current hunk
           ? - print help

Sift through the changes by hunt to get the right one (n - no, pass. y, yes, keep)

...

(1/5) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]?  n 
(2/5) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]?  n
(3/5) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]?  n 

...

     // Update project
     const res2 = await request
-      .patch(`/api/projects/${res.body._id}`)
+      .put(`/api/projects/${res.body._id}`)
       .set(headers)
       .send(updatedDataPayload);
     expect(res2.status).toBe(200);
(4/5) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]?  y // This one!

(5/5) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]?  n

git status should now show a partial staged modification

git status

MM backend/routers/projects.router.test.js
~  <-- This one is Green
 ~ <-- This one is Red

Commit these changes with amend

git commit --amend --no-edit

Finish out the rebase and force push up, overwriting your old commit with the linter changes.

git rebase --continue
git push --force

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll go ahead and fix the linting but I am leaning on scrapping most of this PR since we need to fix the Project Form. We currently need to separate the ProjectFrom component, AddProjectForm, and EditProjectForm component so we can reuse the form component for both the add and edit. It would be much cleaner and remove some unnecessary lines.

.set(headers)
.send(updatedDataPayload);
expect(res2.status).toBe(200);
Expand All @@ -96,15 +87,12 @@ describe('DELETE', () => {
};

// Submit a project
const res = await request
.post('/api/projects/')
.set(headers)
.send(submittedData);
const res = await request.post('/api/projects/').set(headers).send(submittedData);
expect(res.status).toBe(201);

// Delete project
const res2 = await request.patch(`/api/projects/${res.body._id}`).set(headers);
expect(res2.status).toBe(200);
done();
});
});
});
26 changes: 6 additions & 20 deletions client/src/api/ProjectApiService.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,42 +52,28 @@ class ProjectApiService {
console.log('THIS BASEPROJECT URL', this.baseProjectUrl);

try {
const proj = await fetch(this.baseProjectUrl, requestOptions);
const projectDetails = await proj.json()
return projectDetails._id
const proj = await fetch(this.baseProjectUrl, requestOptions);
const projectDetails = await proj.json();
return projectDetails._id;
Copy link
Member

Choose a reason for hiding this comment

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

I hate to be a stickler about this stuff, but can you drop these changes? It makes it a lot harder to follow what is being changed the PR with a lot of other linting changes.

I know it's a hassle and we don't have a linter up and running for the repo yet, but adding the specifc section you're wanting to edit wth git add --patch ${file} or git add -p ${file} and picking the lines of your PR that aren't linting would make it a lot easier to process this.

There's a writeup in another comment on this PR review that has a walkthrough

} catch (error) {
console.error(`Add project error: `, error);
alert('Server not responding. Please try again.');
return undefined;
}
}

async updateProject(projectId, fieldName, fieldValue) {
let updateValue = fieldValue;
// These field are arrays, but the form makes them comma separated strings,
// so this adds it back to db as an arrray.
if (
fieldValue &&
(fieldName === 'partners' || fieldName === 'recruitingCategories')
) {
updateValue = fieldValue
.split(',')
.filter((x) => x !== '')
.map((y) => y.trim());
}

async updateProject(projectId, projectData) {
// Update database
const url = `${this.baseProjectUrl}${projectId}`;
const requestOptions = {
method: 'PATCH',
method: 'PUT',
headers: this.headers,
body: JSON.stringify({ [fieldName]: updateValue }),
body: JSON.stringify({ ...projectData }),
Copy link
Member

@trillium trillium Jul 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If I'm understand this correclty, this change is because we're now submitting a whole JSON object to update the database entry intead of one specific field value. Nice work!

};

try {
const response = await fetch(url, requestOptions);
const resJson = await response.json();

return resJson;
} catch (error) {
console.log(`update project error: `, error);
Expand Down
Loading