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

PIMS-1623/1622: Handling project notifications and tasks #2370

Merged
merged 11 commits into from
May 8, 2024

Conversation

GrahamS-Quartech
Copy link
Contributor

🎯 Summary

PIMS-1623
PIMS-1622

  • Updated the project notification handler with correct logic for calling the notification service to fire emails when a project status update happens.
  • Updated project task handler logic slightly.
  • Implemented a bunch of custom frontend logic to handle showing and selecting tasks for a given status transition when editing the general info section.
  • Revamped the documentation data card section to show a list of tasks the project has gone through previously, grouped by status.

🔰 Checklist

  • I have read and agree with the following checklist and am following the guidelines in our Code of Conduct document.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation where required.
  • I have tested my changes to the best of my ability.
  • My changes generate no new warnings.

… Fixed some logic in Task updating too. Implemented a way to update tasks when transitioning to a new status in the frontend. Revamped the documentation data card to show an immutable list of tasks organized by status.
Copy link

github-actions bot commented May 7, 2024

🚀 Deployment Information

The React APP Image has been built with the tag: 2370. Please make sure to utilize this specific tag when promoting these changes to the TEST and PROD environments during the APP deployment. For more updates please monitor Image Tags Page on Wiki.

Copy link

codeclimate bot commented May 8, 2024

Code Climate has analyzed commit 05d6671 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 95.1%.

View more on Code Climate.

Copy link
Collaborator

@LawrenceLau2020 LawrenceLau2020 left a comment

Choose a reason for hiding this comment

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

I think we're missing the UI to display the different notes: agency, reporting, shared and private notes.

@GrahamS-Quartech
Copy link
Contributor Author

I think we're missing the UI to display the different notes: agency, reporting, shared and private notes.

Yeah, this will just have to be another task kinda like the agency interest responses. Bummer that we sorta missed these in the initial design phase.

Copy link
Collaborator

@dbarkowsky dbarkowsky left a comment

Choose a reason for hiding this comment

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

image
I like that we can see all these now. Might have to confirm if those duplicate Appraise/Approve tasks are needed. We also probably won't show all of these at once in the future, as some projects will never need ERP exemption tasks, etc.

express-api/src/services/projects/projectsServices.ts Outdated Show resolved Hide resolved
await projectRepo.save({ ...project, Metadata: newMetadata });
await projectRepo.save({ ...project, Metadata: newMetadata, Tasks: undefined });
//Seems this save will also try to save Tasks array if present, but if missing the ProjectId it will do weird stuff.
//So we could consolidate handleProjectTasks to here if we wanted, but then it might be annoying trying to get the more specific behavior in that function.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Had this issue on a previous PR. Thought turning cascade off for that relationship would have helped, but I guess not. I think this works good enough.

if (!data || !tasks || !statuses) {
return {};
}
//Somewhat evil reduce where we collect information from the status and tasks lookup so that we can
Copy link
Collaborator

Choose a reason for hiding this comment

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

Evil but clever.

import { Box, Paper, Typography } from '@mui/material';
import React from 'react';

const Forbidden = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good for redirections through the AuthGuard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comitted this by accident... A leftover from when I was trying to do exactly that. We probably actually will want this eventually so I guess I won't get rid of it.

Copy link
Collaborator

@dbarkowsky dbarkowsky left a comment

Choose a reason for hiding this comment

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

Happy with that last change.

@GrahamS-Quartech GrahamS-Quartech merged commit 9955867 into main May 8, 2024
9 checks passed
@GrahamS-Quartech GrahamS-Quartech deleted the DisposalProjectBasicWorkflow branch May 8, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants