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

New fetch logic #1627

Closed
wants to merge 31 commits into from
Closed

Conversation

bkmorgan3
Copy link
Member

@bkmorgan3 bkmorgan3 commented Apr 16, 2024

Fixes #1619

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

This PR might open up a few further questions.
First, Can we guarantee that a User will only PM one project? I hardcoded a fetch based off only 1 ID in a users Managed Projects array. That would introduce a slippery slope of additional requests if there are more than one. if, 2, if 3, etc.

  • I added a check for accessLevel and fetch project info accordingly
  • render the name of the project(s)

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Visuals before changes are applied

Screenshot 2024-04-16 at 3 28 05 PM

Visuals after changes are applied

Screenshot 2024-04-16 at 2 00 58 PM

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 bkmorgan3-new-fetch-logic development
git pull https://github.com/bkmorgan3/VRMS.git new-fetch-logic

@JackHaeg
Copy link
Member

JackHaeg commented Apr 23, 2024

Per Trillium - Changes requested:

  • Drop yarn lock changes
  • Configure fetch call to accept multiple projects (U
    • filter the list server side
    • OR Add query perimeters to the MongoDB call

@bkmorgan3
Copy link
Member Author

@Spiteless I opted for filtering on server because I didn't find a way to use multiple calls to Mongo.
For testing a PM for 2 ids I used this
user.managedProjects.push("6453dfff390bc1002ab3eb6d")
inside the check where user?.accessLevel !== 'admin, line 53ish in ProjectList.

Copy link
Member

@trilliumsmith trilliumsmith left a comment

Choose a reason for hiding this comment

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

Added a few comments, leme know what you think!

@@ -70,7 +67,7 @@ export default function ProjectList() {
}

// Render loading circle until project data is served from API
if (!projects)
if (!project && !projects)
Copy link
Member

Choose a reason for hiding this comment

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

This is the same test twice, was it meant to be something else?

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, I see that its project singular, and projects, multiple. My bad

Copy link
Member

Choose a reason for hiding this comment

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

It would help to change the names of these variables so they're more clear though

@@ -99,7 +96,21 @@ export default function ProjectList() {
)}

<TitledBox title="Active Projects" childrenBoxSx={{ p: 2 }}>
{projects.map((project) => (
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it can be refactorered so that we're not duplicating the component structure.

How do you feel about defining a projectName, projectTo, projectId and maybe projectsList properties in the function body based on the differences in then user?.accessLevel map so we don't need to redefine the component structure?

If you feel like this is more clear as written that's fine too

yarn.lock Outdated
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 restore this file to an unchanged state

@@ -7,6 +7,8 @@ const { AuthUtil } = require("../middleware");
// The base is /api/projects
router.get('/', ProjectController.project_list);

router.put('/', ProjectController.projects)
Copy link
Member

Choose a reason for hiding this comment

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

This feels like we don't want this to a PUT request because the client is only requesting information, not updating anything. If we were to turn this into a GET request what would need to change in order for it to work properly?

trilliumsmith and others added 19 commits May 22, 2024 13:34
* Move generateEventData to its own file
* Add tests for generateEventData
Co-authored-by: danieldkim42 <danieldkim42@users.noreply.github.com>
Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.2 to 1.15.4.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.15.2...v1.15.4)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Changes Team Meetigns to Meetings
@bkmorgan3 bkmorgan3 force-pushed the new-fetch-logic branch 2 times, most recently from fd8d8f4 to cdbf408 Compare May 26, 2024 20:30
@bkmorgan3 bkmorgan3 closed this May 26, 2024
@bkmorgan3
Copy link
Member Author

My rebase / merge game is not tight

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.

Security leak: Rewrite the ProjectList.js page to only call necessary project data when pm's are viewing
7 participants