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-6948 : Create a disposition file #3631

Merged
merged 11 commits into from
Dec 8, 2023
Merged

Conversation

eddherrera
Copy link
Collaborator

image

image

Eduardo Herrera added 4 commits November 30, 2023 20:11
# Conflicts:
#	source/backend/entities/Partials/DispositionStatusTypeCode.cs
#	source/backend/tests/core/Entities/DispositionFileHelper.cs
@eddherrera eddherrera added the enhancement New feature or request label Dec 1, 2023
@eddherrera eddherrera self-assigned this Dec 1, 2023
@devinleighsmith
Copy link
Collaborator

devinleighsmith commented Dec 2, 2023

ui issues:
image
image

image

Please discuss these visual differences with Ana. Personally, I think swapping the x icon out with a trash can should have been handled by another story, as that component is shared by multiple files. Also the text +Add contact... is strictly worse than +Add Team Member...

@devinleighsmith
Copy link
Collaborator

This error means that you need to make these fields nullable in the yup schema:
image

@devinleighsmith
Copy link
Collaborator

adding a property to a disposition file doesn't appear to work. Clicking the drop marker icon on the map doesn't seem to add anything to the list.

[HasPermission(Permissions.DispositionAdd)]
[Produces("application/json")]
[ProducesResponseType(typeof(DispositionFileModel), 200)]
[SwaggerOperation(Tags = new[] { "dispositionfile" })]
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add:
[TypeFilter(typeof(NullJsonResultFilter))]
to ensure that null values are handled appropriately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

}
else
{
if (overrideCodes.Contains(UserOverrideCode.DisposingPropertyNotInventoried))
Copy link
Collaborator

@devinleighsmith devinleighsmith Dec 2, 2023

Choose a reason for hiding this comment

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

some potential to extract this repeated code into a shared method.
(153 - 161

try
{
var foundProperty = _propertyRepository.GetByPid(pid);
dispProperty.PropertyId = foundProperty.Internal_Id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is missing the
UpdateLocation(acquisitionProperty.Property, ref foundProperty, userOverrideCodes);
method that is in the acquisitionfileservice.

What that'll mean is that when a pre-existing property is added to a disposition file it will not be possible to update it's location on the map. Perhaps this is ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

@@ -15,6 +16,11 @@ namespace Pims.Dal.Repositories
/// </summary>
public class DispositionFileRepository : BaseRepository<PimsDispositionFile>, IDispositionFileRepository
{
private const string FILENUMBERPREFIX = "D";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this. The D in front of the file number is for display only, we don't want the D in the database.

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

using var scope = Logger.QueryScope();
disposition.ThrowIfNull(nameof(disposition));

disposition.FileNumber = GeneratetDispositionFileNumber();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Generatet

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

dispositionInitialValues: DispositionFormModel;
loading: boolean;
displayFormInvalid: boolean;
// onSubmit: (dispositionFile: Api_DispositionFile) => Promise<Api_DispositionFile | undefined>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove if not needed.

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
Collaborator

@devinleighsmith devinleighsmith left a comment

Choose a reason for hiding this comment

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

need the properties selector to add properties to the form.

Eduardo Herrera added 4 commits December 4, 2023 12:10
# Conflicts:
#	source/backend/api/Areas/Disposition/Controllers/DispositionFileController.cs
#	source/backend/api/Services/IDispositionFileService.cs
#	source/backend/dal/Repositories/DispositionFileRepository.cs
#	source/frontend/src/constants/API.ts
#	source/frontend/src/hooks/pims-api/useApiDispositionFile.ts
#	source/frontend/src/hooks/repositories/useDispositionProvider.ts
@eddherrera
Copy link
Collaborator Author

Fixed issues, pending to review with Ana

@eddherrera eddherrera marked this pull request as ready for review December 5, 2023 18:55
# Conflicts:
#	source/backend/api/Areas/Disposition/Controllers/DispositionFileController.cs
#	source/backend/apimodels/Models/Concepts/DispositionFile/DispositionFileMap.cs
#	source/backend/dal/Exceptions/OverrideExceptions.cs
@eddherrera eddherrera merged commit 6875779 into bcgov:disposition Dec 8, 2023
1 check passed
@eddherrera eddherrera deleted the psp-6948 branch December 14, 2023 22:04
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