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

Feat/cwu review #430

Merged
merged 11 commits into from
Nov 8, 2023
Merged

Feat/cwu review #430

merged 11 commits into from
Nov 8, 2023

Conversation

bdolor
Copy link
Collaborator

@bdolor bdolor commented Nov 6, 2023

This PR closes issue: [DMM-300]

Includes tests? [N]
Updated docs? [N]

Proposed changes:

  • adds 'under review' status to CWU
  • includes notifications

@bdolor bdolor requested a review from IanFonzie November 6, 2023 20:26
IanFonzie
IanFonzie requested changes Nov 6, 2023
View reviewed changes
Copy link
Collaborator

@IanFonzie IanFonzie left a comment

Choose a reason for hiding this comment

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

Good work; the front-end has a couple of testing blindspots though. There's a few small issues regarding toasts that I mentioned in the other comments, but a bigger issue is that we're still rendering the "Actions" button for government users after the opportunity is published, which differs from SWU.

@bdolor
Copy link
Collaborator Author

bdolor commented Nov 7, 2023

...but a bigger issue is that we're still rendering the "Actions" button for government users after the opportunity is published, which differs from SWU.

sounds like this is something that wasn't introduced in this PR, but may still need to be addressed?

@bdolor bdolor requested a review from IanFonzie November 7, 2023 00:29
IanFonzie
IanFonzie requested changes Nov 7, 2023
View reviewed changes
src/back-end/lib/resources/opportunity/code-with-us.ts Outdated
Copy link
Collaborator

@IanFonzie IanFonzie Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we need to add a publish permissions check on line 750 because users can publish their own opportunities as is

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should be addressed in b6c904c

@bdolor bdolor requested a review from IanFonzie November 8, 2023 00:36
IanFonzie
IanFonzie approved these changes Nov 8, 2023
View reviewed changes
Copy link
Collaborator

@IanFonzie IanFonzie left a comment

Choose a reason for hiding this comment

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

Looks good!

@bdolor bdolor merged commit 029d515 into development Nov 8, 2023
1 check passed
@bdolor bdolor deleted the feat/cwu-review branch November 8, 2023 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IanFonzie IanFonzie IanFonzie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bdolor @IanFonzie