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-7295 : Create/Edit Appraisal and Assessment #3709

Merged
merged 9 commits into from
Jan 15, 2024

Conversation

eddherrera
Copy link
Collaborator

image

# Conflicts:
#	source/backend/api/Services/DispositionFileService.cs
#	source/backend/apimodels/Models/Concepts/DispositionFile/DispositionFileAppraisalModel.cs
Copy link
Contributor

✅ No secrets were detected in the code.

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

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

Comparison is base (69b93b4) 75.11% compared to head (0c0794c) 74.91%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #3709      +/-   ##
==========================================
- Coverage   75.11%   74.91%   -0.21%     
==========================================
  Files        1435     1439       +4     
  Lines       36957    37184     +227     
  Branches     6928     6973      +45     
==========================================
+ Hits        27762    27855      +93     
- Misses       8917     9051     +134     
  Partials      278      278              
Flag Coverage Δ
unittests 74.91% <40.57%> (-0.21%) ⬇️

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

Files Coverage Δ
...pts/DispositionFile/DispositionFileAppraisalMap.cs 100.00% <100.00%> (ø)
...tion/tabs/offersAndSale/OffersAndSaleContainer.tsx 72.41% <100.00%> (+3.18%) ⬆️
...Appraisal/form/DispostionAppraisalFormYupSchema.ts 100.00% <100.00%> (ø)
...rc/features/projects/reports/ProjectExportForm.tsx 100.00% <ø> (ø)
source/frontend/src/utils/numberFormatUtils.ts 100.00% <100.00%> (ø)
...apSideBar/disposition/router/DispositionRouter.tsx 50.00% <0.00%> (-8.34%) ⬇️
.../tabs/offersAndSale/OffersAndSaleContainerView.tsx 85.71% <75.00%> (-4.61%) ⬇️
...ontend/src/hooks/pims-api/useApiDispositionFile.ts 26.66% <0.00%> (-2.97%) ⬇️
...d/src/hooks/repositories/useDispositionProvider.ts 65.85% <50.00%> (-2.72%) ⬇️
...sal/update/UpdateDispositionAppraisalContainer.tsx 83.87% <83.87%> (ø)
... and 6 more

@devinleighsmith
Copy link
Collaborator

@eddherrera your implementation, confluence, and uxpin don't seem to match for this story. Please ask Julian/Ana to correct their work so that it is consistent, and then make sure your story is in alignment.

@devinleighsmith
Copy link
Collaborator

continuing to type into the year date input causes the app to crash.

@devinleighsmith
Copy link
Collaborator

image

@devinleighsmith
Copy link
Collaborator

getting this whenever I try and save.

[ProducesResponseType(typeof(DispositionFileAppraisalModel), 200)]
[SwaggerOperation(Tags = new[] { "dispositionfile" })]
[TypeFilter(typeof(NullJsonResultFilter))]
public IActionResult UpdateDispositionFileAppraisal([FromRoute] long id, [FromRoute] long appraisalId, [FromBody] DispositionFileAppraisalModel dispositionFileAppraisal)
Copy link
Collaborator

Choose a reason for hiding this comment

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

To co-exist with our existing patterns I'd be tempted to remove the add endpoint entirely. In general when editing sub-entities we consider that an update. We only use post endpoints for first class entities like files or persons. sub-entities are all handled with puts, and that single put handles add/edit/delete (allthough in this case it'll just handle add/edit).

return _dispositionFileRepository.GetDispositionFileAppraisal(dispositionFileId);
}

public PimsDispositionAppraisal AddDispositionFileAppraisal(long dispositionFileId, PimsDispositionAppraisal dispositionAppraisal)
Copy link
Collaborator

Choose a reason for hiding this comment

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

see other comment on add/update.

var dispositionFileParent = _dispositionFileRepository.GetById(dispositionFileId);
if (dispositionFileId != dispositionAppraisal.DispositionFileId || dispositionAppraisal.DispositionAppraisalId != appraisalId || dispositionFileParent is null)
{
throw new BadRequestException("Invalid dispositionFileId.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

normally we do this at the repository level in my experience.

)}
claim={Claims.DISPOSITION_EDIT}
key={'disposition'}
title={'Add Disposition Offer'}
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 title correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Udpated

@devinleighsmith
Copy link
Collaborator

Would like to see tests for the controller and repository methods added.

Copy link
Contributor

✅ No secrets were detected in the code.

Copy link
Contributor

✅ No secrets were detected in the code.

# Conflicts:
#	source/backend/api/Services/DispositionFileService.cs
#	source/frontend/src/features/mapSideBar/disposition/router/DispositionRouter.tsx
Copy link
Contributor

✅ No secrets were detected in the code.

Copy link
Contributor

✅ No secrets were detected in the code.

@eddherrera eddherrera merged commit 4bd5561 into bcgov:dev Jan 15, 2024
9 checks passed
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.

2 participants