-
Notifications
You must be signed in to change notification settings - Fork 7
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
Feature verification #2613
Feature verification #2613
Conversation
@@ -428,13 +428,12 @@ export default class ProjectsController { | |||
|
|||
static async editLinkType(req: Request, res: Response, next: NextFunction) { | |||
try { | |||
const { linkId } = req.params; | |||
const { iconName, required, linkTypeName } = req.body; | |||
const { linkTypeName } = req.params; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why aren't we using the linkTypeId here
wbsProposedChanges Wbs_Proposed_Changes @relation(name: "wpProposedChanges", fields: [wbsProposedChangesId], references: [wbsProposedChangesId]) | ||
wbsProposedChangesId String @unique | ||
projectProposedChanges Project_Proposed_Changes? @relation(fields: [projectProposedChangesId], references: [projectProposedChangesId]) | ||
projectProposedChangesId String? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can u explain what this addition/ all the code related to this is for from a high level
|
||
carsRouter.get('/', CarsController.getAllCars); | ||
|
||
carsRouter.post('/create', CarsController.createCar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no validation?
@@ -176,9 +175,6 @@ export default class ChangeRequestsService { | |||
// reviews a proposed solution applying certain changes based on the content of the proposed solution | |||
await reviewProposedSolution(psId, foundCR, reviewer, organizationId); | |||
} else if (foundCR.scopeChangeRequest?.wbsProposedChanges && !psId) { | |||
// we don't want to have merge conflictS on the wbs element thus we check if there are unreviewed or open CRs on the wbs element | |||
await validateNoUnreviewedOpenCRs(foundCR.wbsElementId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should still be here even if it's also on all the creates just to be safe, unless there's a specific reason you removed it
where: { uniqueExpenseType: { name, organizationId } } | ||
}); | ||
|
||
if (existingAccount && existingAccount.dateDeleted) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a comment about what we're doing here, it makes sense, but isn't immediately obvious
); | ||
if (workPackagesToAdd.length === 0) continue; | ||
project.workPackages.push(...workPackagesToAdd); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments in the code u added here and above would be helpful. I understand the code in here before was pretty bad/ needs comments as well lol, apologies about that, was not expecting to be developing on it so soon
eventId: newProject.id, | ||
type: 'create-project' | ||
}); | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as that other inline, this is pretty complex if we could move it above the return and give it a good name/ a high level comment, that'd be good
try { | ||
validateWBS(change.eventId); | ||
return <GanttTimeLineChangeModal change={change} handleClose={handleClose} open={open} />; | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't love this, if there's a way to add the change type to RequestEventChange that'd be better, but the gantt chart code is pretty fucked, so if need be just make a ticket for it, and we can clean it up later
))} | ||
</ul> | ||
</> | ||
))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't really test this because my local finishline is being weird and not loading rn, but I'm sure it's good
baseWbs: | ||
updatedEvent.type === 'project' | ||
? { carNumber: updatedEvent.carNumber, projectNumber: 0, workPackageNumber: 0 } | ||
: { carNumber: updatedEvent.carNumber, projectNumber: updatedEvent.projectNumber, workPackageNumber: 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I guess this is where the type would be added, if we could move the weird logic here, or even change the code to flow better so we could directly put the type of change from the submitted event changes to this, that'd be awesome. But as I type that out it probably feels best to just do it in another ticket, probably good for one of the tech leads so they get experience looking thru this code/ cleaning shit up
Changes
Verifying Existing Features and Adding Necessary Additional Features
Checklist
It can be helpful to check the
Checks
andFiles changed
tabs.Please review the contributor guide and reach out to your Tech Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
yarn.lock
changes (unless dependencies have changed)Closes # (issue #)