-
Notifications
You must be signed in to change notification settings - Fork 24
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
Acquisition File Header | psp-6936 #3507
Conversation
✅ No secrets were detected in the code. |
1 similar comment
✅ No secrets were detected in the code. |
Codecov Report
@@ Coverage Diff @@
## dev #3507 +/- ##
==========================================
- Coverage 71.25% 70.44% -0.82%
==========================================
Files 1341 1341
Lines 31969 32369 +400
Branches 6012 6019 +7
==========================================
+ Hits 22781 22802 +21
- Misses 8941 9320 +379
Partials 247 247
Flags with carried forward coverage won't be shown. Click here to find out more.
|
mild issue, but worth mentioning, when I modify properties on an acq file I see two requests to updateinfo. |
doesn't seem to affect all entities, checklist updates only fire one updateinfo request. |
The AC specifies that form8 add update/delete should trigger this but I'm not seeing the request fire. |
@FuriousLlama same thing with takes, doesn't seem to be wired up. |
.ToList(); | ||
lastUpdatedByAggregate.AddRange(expPaymentsHistoryLastUpdatedBy); | ||
|
||
// Acquisition Compensation Requsition Financials |
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.
nit: comment.
.ToList(); | ||
lastUpdatedByAggregate.AddRange(expPaymentsItemsLastUpdatedBy); | ||
|
||
// Acquisition Deleted Compensation Requsition Financials |
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.
nit: comment
/// </summary> | ||
/// <param name="id"></param> | ||
/// <returns></returns> | ||
public LastUpdatedByModel GetLastUpdateBy(long id) |
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.
The size of this is a bit of a code smell, but I feel like refactoring this to use some kind of generic method might also be messy. Thoughts?
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 agree. For now I rather leave it as is and create a functor later.
@@ -10,7 +10,7 @@ export const TakesYupSchema = Yup.object().shape({ | |||
takes: Yup.array().of( | |||
Yup.object().shape({ | |||
description: Yup.string().max(4000, 'Description must be at most ${max} characters'), | |||
takeTypeCode: Yup.string().required('Take type is required'), | |||
takeTypeCode: Yup.string().required('Take type is required').nullable(), |
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.
hmm?
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 fixes an issue with the take validation where it was showing an error instead of the message
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.
just one weird line there in they yup schema that I wasn't expecting, and then those two sub-entities that I mentioned.
✅ No secrets were detected in the code. |
✅ No secrets were detected in the code. |
No description provided.