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

update edit page 1396 #1401

Closed

Conversation

plang-psm
Copy link
Member

Fixes #1396

What changes did you make and why did you make them ?

  • Updated the styling to match the Figma document.
  • Updated the PATCH request to accept all changes of the form instead of individual changes.
  • ** Radio buttons do not save -- see below

Issue:

The location radio buttons do not have a field in the DB causing us to lose the value when transferring data to the edit page.

My suggestion:

Add another field to the projects collection in the DB so we can set/update the value and pass it through to the radio buttons. This would need to be done in the Add Project Form before adding to the Edit Form.

Extra

I left some state/change solution that could be used if needed when the time comes. Also, we should explore making a reusable Form component that takes in an array of inputs to keep @MattPereira 's great styling!

Visuals before changes are applied

Old edit page

Visuals after changes are applied

New edit page

@github-actions
Copy link

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b plang-psm-edit-page-update-1396 development
git pull https://github.com/plang-psm/VRMS.git edit-page-update-1396

Copy link
Member

@bkmorgan3 bkmorgan3 left a comment

Choose a reason for hiding this comment

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

@plang-psm I left a comment down below but marking this reviewed, officially.

<Divider sx={{ borderColor: 'rgba(0,0,0,1)' }} />
<Box sx={{ py: 2, px: 4 }}>
<form id="project-form" onSubmit={handleSubmit}>
{simpleInputs.map((input, event) => (
Copy link
Member

Choose a reason for hiding this comment

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

@plang-psm
1st - Good job this works as expected
2- Not sure if this is our decision maybe we can talk this out with design. I think there should be an edit button to make the fields editable so a user knows they are in Edit Mode.
3- Can any Admin user edit any existing project?

Copy link
Member

Choose a reason for hiding this comment

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

Also would the Patch request be a Put since the entire Project Object is being submitted?

Copy link
Member Author

@plang-psm plang-psm Jun 21, 2023

Choose a reason for hiding this comment

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

Good catch @bkmorgan3 . You are correct, it should be a PUT instead of a PATCH because I am submitting all the data. I went ahead and updated it.

In regards to 2, I just went with whatever was on the Figma design but I know that is a feature we did talk about once. Wasn't sure if it was decided on so I just applied the UI.

In regards to 3, I am not completely sure. From my understanding only admin and project managers on that specific team (?) are able to view the projects and edit them. I don't think users or PMs not on that team have access to them.

Copy link
Member

@trillium trillium Jul 1, 2023

Choose a reason for hiding this comment

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

  1. Yeah, this is good work.

  2. I agree we should look for an indication that the project is being edited. I'll ping the design team about that.

@juliagab56 Can you wiegh in on this?

Seems like it should go here - there's a spot for it where the Add Project SVG is:

Add Project

image

Edit Project

image

Disregard all the little letters in the boxes, that's an extension of mine

Copy link
Member

@trillium trillium left a comment

Choose a reason for hiding this comment

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

This is well on its way!

Stoked to see this coming together, thanks for getting the database update right!

@bkmorgan3 has an open PR as well that's adding in form validation for the AddProject component, and I think we'll end up pulling some of his work into this, but I think once we get rid of the additional lint changes this can be merged in!

@@ -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

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.

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

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

@trillium trillium Jul 1, 2023

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!

<Divider sx={{ borderColor: 'rgba(0,0,0,1)' }} />
<Box sx={{ py: 2, px: 4 }}>
<form id="project-form" onSubmit={handleSubmit}>
{simpleInputs.map((input, event) => (
Copy link
Member

@trillium trillium Jul 1, 2023

Choose a reason for hiding this comment

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

  1. Yeah, this is good work.

  2. I agree we should look for an indication that the project is being edited. I'll ping the design team about that.

@juliagab56 Can you wiegh in on this?

Seems like it should go here - there's a spot for it where the Add Project SVG is:

Add Project

image

Edit Project

image

Disregard all the little letters in the boxes, that's an extension of mine

@@ -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

@@ -313,7 +312,7 @@ textarea {
}

.event-list-details {
display: flex;
display: flex;
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

Copy link
Member

Choose a reason for hiding this comment

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

Easiest way to distinguish rn between editable content vs only view would be to make use of the form field when editing and have no element around the information when only viewed
ie:
Screenshot 2023-07-03 at 10 25 08 AM

You'd have to disregard the "[edit]" buttons. Not sure what the feedback on that was but we can revisit the idea of only having one button for editing instead of one for each field. I can have some updated designs for you all later this afternoon

@trillium
Copy link
Member

trillium commented Jul 1, 2023

@plang-psm

re:

Issue:

The location radio buttons do not have a field in the DB causing us to lose the value when transferring data to the edit page.

Here's the model of the location as we have it now:

const projectSchema = mongoose.Schema({
    name: { type: String },
    description: { type: String },
    githubIdentifier: { type: String },
    projectStatus: { type: String },                    // Active, Completed, or Paused

    location: { type: String },                         // DTLA, Westside, South LA, or Remote (hacknight)

    //teamMembers: { type: String },                    // commented since we should be able to get this from Project Team Members table
    createdDate: { type: Date, default: Date.now },     // date/time project was created
    completedDate: { type: Date },                      // only if Status = Completed, date/time completed
    githubUrl: { type: String },                        // link to main repo
    slackUrl: { type: String },                         // link to Slack channel
    googleDriveUrl: { type: String },
    googleDriveId: { type: String },
    hflaWebsiteUrl: { type: String },
    videoConferenceLink: { type: String },
    lookingDescription: { type: String },               // narrative on what the project is looking for
    recruitingCategories: [{ type: String }],           // same as global Skills picklist
    partners: [{ type: String }],                       // any third-party partners on the project, e.g. City of LA
    managedByUsers: [{ type: String }]                  // Which users may manage this project.                 
});

What do you think about infering whether it's in person or remote based on whether the value passes some test to confirm that it's a valid web URL?

FreeCodeCamp Article

const isValidUrl = urlString=> {
      try { 
      	return Boolean(new URL(urlString)); 
      }
      catch(e){ 
      	return false; 
      }
  }

If it passes that test, start off the radio button with remote, otherwise indicate in-person?

This way we wouldn't have to make changes to the db given that the plan is to move to another backend eventually.

@juliagab56
Copy link
Member

Figma file

@plang-psm
Copy link
Member Author

@plang-psm

re:

Issue:

The location radio buttons do not have a field in the DB causing us to lose the value when transferring data to the edit page.

Here's the model of the location as we have it now:

const projectSchema = mongoose.Schema({
    name: { type: String },
    description: { type: String },
    githubIdentifier: { type: String },
    projectStatus: { type: String },                    // Active, Completed, or Paused

    location: { type: String },                         // DTLA, Westside, South LA, or Remote (hacknight)

    //teamMembers: { type: String },                    // commented since we should be able to get this from Project Team Members table
    createdDate: { type: Date, default: Date.now },     // date/time project was created
    completedDate: { type: Date },                      // only if Status = Completed, date/time completed
    githubUrl: { type: String },                        // link to main repo
    slackUrl: { type: String },                         // link to Slack channel
    googleDriveUrl: { type: String },
    googleDriveId: { type: String },
    hflaWebsiteUrl: { type: String },
    videoConferenceLink: { type: String },
    lookingDescription: { type: String },               // narrative on what the project is looking for
    recruitingCategories: [{ type: String }],           // same as global Skills picklist
    partners: [{ type: String }],                       // any third-party partners on the project, e.g. City of LA
    managedByUsers: [{ type: String }]                  // Which users may manage this project.                 
});

What do you think about infering whether it's in person or remote based on whether the value passes some test to confirm that it's a valid web URL?

FreeCodeCamp Article

const isValidUrl = urlString=> {
      try { 
      	return Boolean(new URL(urlString)); 
      }
      catch(e){ 
      	return false; 
      }
  }

If it passes that test, start off the radio button with remote, otherwise indicate in-person?

This way we wouldn't have to make changes to the db given that the plan is to move to another backend eventually.

I like the idea but testing to set a radio button every time you load a page may hinder the site's performance. We may be able to use mongoDBs .updateMany to set another field in our project objects in our DB and just set it as false? My only concern would be that all the projects would still need to be updated since the default would be false. We could have PMs go in an update that field or I am open to other ideas.

@plang-psm
Copy link
Member Author

Love the design! My only concern would be that we would need to use one of the two styles if we want to have a reusable project form component. An alternative may be to grey out the fields when the edit button is disabled and removing it when in "edit mode". What do you think @juliagab56 @spiteless

@juliagab56
Copy link
Member

@plang-psm I'm confused as to what's going on since I keep having different discussions on other issues about similar things. But in this case, it's personally fine with me if you want to make these small design decisions that will move the issue forward. The greyed out fields sounds like a good idea to me as a quick solution. I am working on having a latest set of wireframes with the most updated screens but I need to be able to align with devs to understand what stage we are at to do so. I am also missing some reqs and specs I'm gathering.

@jbubar
Copy link
Member

jbubar commented Jul 28, 2023

This looks good to me. We will bring it up in the next monday meeting and Ill try to get it merged. Thanks for all your work @plang-psm

@trillium
Copy link
Member

trillium commented Aug 1, 2023

Talk on this PR from todays meeting:

@jbubar

  • Recurring events are the most important field, providing users that information first would be better
  • Save button is currently below the recurring events, might seem confusing to the user
  • Editing and updating recurring
  • This PR improves look and feel regardless

Moving forward, we should

  • Merge this PR
  • Create separate PR to move where the save buttons are

@spiteless

  • This PR still needs indication to the user that the form is in an edit state
  • Make a new issue to adapt this design to accept the disabled prop, allow fields to become editable on click of disabled field, indicate to the user that the form is in an edit state via edit badge to the direct right of the Project Information h2

@plang-psm
Copy link
Member Author

see #1471

@plang-psm plang-psm closed this Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply new Figma to the edit projects page
5 participants