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

PSP-7618 : Re-add project/product to Disposition Files (Create, Update and View File Summary) #3777

Merged
merged 14 commits into from
Feb 8, 2024

Conversation

eddherrera
Copy link
Collaborator

image

image

@eddherrera eddherrera added the enhancement New feature or request label Feb 7, 2024
@eddherrera eddherrera self-assigned this Feb 7, 2024
Copy link
Contributor

github-actions bot commented Feb 7, 2024

✅ No secrets were detected in the code.

Copy link
Contributor

github-actions bot commented Feb 7, 2024

✅ No secrets were detected in the code.

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: 69 lines in your changes are missing coverage. Please review.

Comparison is base (354e7ff) 77.54% compared to head (43401a1) 75.65%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #3777      +/-   ##
==========================================
- Coverage   77.54%   75.65%   -1.90%     
==========================================
  Files         493     1448     +955     
  Lines       17099    37910   +20811     
  Branches     1122     7133    +6011     
==========================================
+ Hits        13260    28682   +15422     
- Misses       3560     8945    +5385     
- Partials      279      283       +4     
Flag Coverage Δ
unittests 75.65% <55.76%> (-1.90%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...rce/backend/api/Services/DispositionFileService.cs 85.56% <ø> (ø)
...els/Concepts/DispositionFile/DispositionFileMap.cs 100.00% <100.00%> (ø)
...Concepts/DispositionFile/DispositionFileSaleMap.cs 100.00% <100.00%> (ø)
...ispositionFile/DispositionSalePurchaserAgentMap.cs 100.00% <ø> (ø)
...sitionFile/DispositionSalePurchaserSolicitorMap.cs 100.00% <ø> (ø)
source/backend/dal/Repositories/BaseRepository.cs 100.00% <ø> (ø)
...tionSale/form/DispositionSalePurchasersSubForm.tsx 94.73% <100.00%> (ø)
...SideBar/disposition/models/DispositionFormModel.ts 91.46% <87.50%> (ø)
.../disposition/models/DispositionSaleContactModel.ts 73.33% <50.00%> (ø)
...Bar/disposition/models/DispositionSaleFormModel.ts 88.76% <80.00%> (ø)
... and 5 more

... and 947 files with indirect coverage changes

public long? ProjectId { get; set; }

/// <summary>
/// get/set - The acquisition project.
Copy link
Collaborator

Choose a reason for hiding this comment

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

acquisition -> disposition

@@ -1,5 +1,6 @@
using System;
using System.Security.Claims;
using Microsoft.EntityFrameworkCore.Storage;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks unused to me (remove)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

{
Context.Remove(existingSale.DspPurchAgent);
dispositionSale.DspPurchAgentId = null;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test coverage on these conditionals is a bit spotty, can you add some tests for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@@ -108,7 +112,7 @@ describe('Disposition List View', () => {
it('searches by pid', async () => {
let results = mockPagedResults([
{
...mockDispositionFileResponse(),
...(mockDispositionFileResponse() as unknown as Api_DispositionFile),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble understanding the need for this, if the entire point of this mock is to create a disposition file, why are you needing to cast this to an Api_DispositionFile via an unknown?

Copy link
Collaborator Author

@eddherrera eddherrera Feb 7, 2024

Choose a reason for hiding this comment

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

I am casting from the new generated models ApiGen_Concepts_XXX I am limiting the use of the new models scoped to what I am currently working and not trying to spill over everywhere. The story to do the removal of old models would just need to remove the casting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah ok. I see. That is fine for now then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, can you log a follow-up task for is74 that would cover correcting this once we merge 5.1

That way we don't forget.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Task created.

if (param[0].id) {
const result = await retrieveProjectProducts(param[0].id);
if (result) {
setProjectProducts(result as unknown as ApiGen_Concepts_Product[]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't understand why all this casting is necessary. Is this not the type returned by the API?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as previous comment. Related to the use of the new generated models but limiting to the scope of what I am working on.

);

useEffect(() => {
console.log(initialValues.project);
Copy link
Collaborator

Choose a reason for hiding this comment

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

console.log

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

onChange={(vals: IAutocompletePrediction[]) => {
onMinistryProjectSelected(vals);
if (vals.length === 0) {
formikProps.setFieldValue('project', 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this correct? Or should this be product?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it should be product. updated.

Copy link
Contributor

github-actions bot commented Feb 7, 2024

✅ No secrets were detected in the code.

);

useEffect(() => {
console.log(initialValues.project);
Copy link
Collaborator

Choose a reason for hiding this comment

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

console.log

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

Copy link
Contributor

github-actions bot commented Feb 7, 2024

✅ No secrets were detected in the code.

@devinleighsmith
Copy link
Collaborator

@eddherrera removing a purchaser solicitor or purchaser agent causes the db to throw a foreign key violation exception. Are you missing unit tests for that case?

@devinleighsmith
Copy link
Collaborator

SqlException: *** [PIMS_DSPPUR_I_S_U_TR], Line 14. Errno 547: The UPDATE statement conflicted with the FOREIGN KEY constraint "PIM_DSPSAL_PIM_DSPPUR_FK". The conflict occurred in database "pims", table "dbo.PIMS_DISPOSITION_SALE", column 'DISPOSITION_SALE_ID'.

@devinleighsmith
Copy link
Collaborator

Similarly, I received the following with this test:

  1. Add two purchaser names
  2. Delete one
  3. Update the other

SqlException: *** [PIMS_DSPPUR_I_S_U_TR], Line 14. Errno 547: The UPDATE statement conflicted with the FOREIGN KEY constraint "PIM_DSPSAL_PIM_DSPPUR_FK". The conflict occurred in database "pims", table "dbo.PIMS_DISPOSITION_SALE", column 'DISPOSITION_SALE_ID'.

@devinleighsmith
Copy link
Collaborator

devinleighsmith commented Feb 7, 2024

Also, the "product" field does not seem to be cleared appropriately when the parent project is removed:

  1. Add a project and product.
  2. Remove the project
  3. Add another, different project.
    (Unexpectedly, the product I chose for 1 is being inserted when I select the new project in step 3)

Copy link
Contributor

github-actions bot commented Feb 8, 2024

✅ No secrets were detected in the code.

Copy link
Contributor

github-actions bot commented Feb 8, 2024

✅ No secrets were detected in the code.

Copy link
Contributor

github-actions bot commented Feb 8, 2024

✅ No secrets were detected in the code.

@eddherrera
Copy link
Collaborator Author

@devinleighsmith updated issue with project-product and fixed issue with FK.

Copy link
Contributor

github-actions bot commented Feb 8, 2024

✅ No secrets were detected in the code.

Copy link
Contributor

github-actions bot commented Feb 8, 2024

✅ No secrets were detected in the code.

@eddherrera eddherrera merged commit 04b3dd0 into bcgov:dev Feb 8, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants