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

Linking ATS numbers to related enquiries #257

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

wilwong89
Copy link
Contributor

@wilwong89 wilwong89 commented Feb 5, 2025

Description

Adding logic to EnquiryView and EnquiryForm components to handle ATS numbers relating to enquiries with related activities.
Prevent enquiries with related activities from changing ATS client numbers or creating new ATS client entries
Show ATS client numbers from related activities if available.
PADS-366
PADS-367

Types of changes

New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Copy link

github-actions bot commented Feb 5, 2025

Coverage Report (Application)

Totals Coverage
Statements: 37.03% ( 1103 / 2979 )
Methods: 26.82% ( 136 / 507 )
Lines: 48.73% ( 727 / 1492 )
Branches: 24.49% ( 240 / 980 )

Copy link

github-actions bot commented Feb 5, 2025

Coverage Report (Frontend)

Totals Coverage
Statements: 45.59% ( 3140 / 6887 )
Methods: 34.82% ( 413 / 1186 )
Lines: 53.56% ( 1821 / 3400 )
Branches: 39.37% ( 906 / 2301 )

@wilwong89 wilwong89 force-pushed the feature/related-enquiry-ats branch 4 times, most recently from 8b35806 to addbedc Compare February 5, 2025 21:41
@wilwong89 wilwong89 marked this pull request as ready for review February 5, 2025 22:47
@Subin1Doo
Copy link

The modal that shows up when you click on "search ATS" seems broken..? like the UI and all the layout. New ATS Client modal looks fine.

And just some minor tweaks: When it says Client #: Unavailable, can we get rid of the text underline for the word "unavailable."

And when clicking on the linked ATS client ID from a related enquiry view, the modal buttons are greyed out, which is good, but when you hover over the "save" button it shows the text underline, can we remove that underline as well?

Thank you!

@wilwong89
Copy link
Contributor Author

The modal that shows up when you click on "search ATS" seems broken..? like the UI and all the layout. New ATS Client modal looks fine.

And just some minor tweaks: When it says Client #: Unavailable, can we get rid of the text underline for the word "unavailable."

And when clicking on the linked ATS client ID from a related enquiry view, the modal buttons are greyed out, which is good, but when you hover over the "save" button it shows the text underline, can we remove that underline as well?

Thank you!

Looks like the Primevue update from a while ago silently changed the styling and we didn't catch it. Fixed now.
Styling added to remove underlines when appropriate.

@@ -108,6 +125,15 @@ const onAssigneeInput = async (e: IInputEvent) => {
}
};

const showUserLinkModelCheck = (values: Enquiry) => {
if (relatedAtsNumber || values.atsClientNumber) return true;
Copy link
Collaborator

@qhanson55 qhanson55 Feb 6, 2025

Choose a reason for hiding this comment

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

reason why not returning false? could we just have
return (relatedAtsNumber || values.atsClientNumber)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified.

else false;
};

const handleDetailsModalClick = computed(() => (values: Enquiry) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't think you need computed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

:disabled="!editable"
@click="atsUserLinkModalVisible = true"
>
Search ATS
Copy link
Collaborator

Choose a reason for hiding this comment

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

i18n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with i18n changes.

:disabled="!editable"
@click="atsUserCreateModalVisible = true"
>
New ATS Client
Copy link
Collaborator

Choose a reason for hiding this comment

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

i18n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with i18n changes.

@wilwong89 wilwong89 force-pushed the feature/related-enquiry-ats branch 2 times, most recently from b10e9f7 to 911f0b3 Compare February 6, 2025 21:19
Copy link

github-actions bot commented Feb 6, 2025

Copy link
Member

@jujaga jujaga left a comment

Choose a reason for hiding this comment

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

Putting temporary block while we get confirmation about naming between "number" and "id" from vFCBC and if there are any constraints/business rules around the naming.

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.

4 participants