PSP-7247 : View disposition file (offers and sale) #3669
Conversation
eddherrera
commented
Dec 18, 2023
eddherrera
commented
eddherrera
requested review from
FuriousLlama,
asanchezr and
devinleighsmith
|
@eddherrera the order of fields in the sales section doesn't seem to match the mockup. Can you please check in with Ana and see if either you or her needs to update their work to match? |
| [ProducesResponseType(typeof(IEnumerable<DispositionFileSaleModel>), 200)] | ||
| [SwaggerOperation(Tags = new[] { "dispositionfile" })] | ||
| [TypeFilter(typeof(NullJsonResultFilter))] | ||
| public IActionResult GetDispositionFileSales([FromRoute]long id) |
There was a problem hiding this comment.
Hmm, not sure I agree with this, if this is mapped as a 0-1 I think I would rather this return an object or null, returning an array like this isn't clear to the consumer what the data model actually is.
| @@ -48,6 +51,18 @@ export const DispositionRouter: React.FC<IDispositionRouterProps> = props => { | |||
| <Route path={`${stripTrailingSlash(path)}/property`}> | |||
| <></> | |||
| </Route> | |||
| <AppRoute | |||
| exact | |||
| path={`${stripTrailingSlash(path)}/${FileTabType.OFFERS_AND_SALE}/addOffer`} | |||
There was a problem hiding this comment.
I'm pretty sure we are going to want something like:
${stripTrailingSlash(path)}/${FileTabType.OFFERS_AND_SALE}/offers/new
(new seems to be our standard naming)
why? because for edit we will want to include the specific id of the offer that is being edited, and we want our create/edit paths to follow a similar format, ie:
${stripTrailingSlash(path)}/${FileTabType.OFFERS_AND_SALE}/offers/1
There was a problem hiding this comment.
will fix this on my next PR.
| ]); | ||
|
|
||
| if (offersResponse) { | ||
| setDispositionOffers(offersResponse); |
There was a problem hiding this comment.
nit: could simplify this to just get the data object that is returned by the api request wrapper instead of needing two additional state vars.
There was a problem hiding this comment.
I am using state vars to hold the list of offers and be able to update the offers state once the delete offers succeds.
| dispositionSale: Api_DispositionFileSale | null; | ||
| } | ||
|
|
||
| const OffersAndSaleContainerView: React.FunctionComponent<IOffersAndSaleContainerViewProps> = ({ |
There was a problem hiding this comment.
nit containerview -> view
all containers have views, so no need to include container in the naming of a view.
|
@eddherrera approved, but please make requested frontend/backend route changes. |
| @@ -1,6 +1,7 @@ | |||
| using System.Collections.Generic; | |||
| using System.Linq; | |||
| using System.Security.Claims; | |||
| using DocumentFormat.OpenXml.Office2010.Excel; | |||
There was a problem hiding this comment.
most likely this import can be safely removed...
There was a problem hiding this comment.
I have no idea how this keeps getting included.
|
|
||
| IEnumerable<PimsDispositionOffer> GetOffers(long dispositionFileId); | ||
|
|
||
| IEnumerable<PimsDispositionSale> GetSales(long dispositionFileId); |
There was a problem hiding this comment.
Agree with Devin that this should return a single instance of PimsDispositionSale (or null)
There was a problem hiding this comment.
I am a little conflicted with this because the Entity Classes from the Scaffolding do not declare the relationship as such.
| namespace Pims.Dal.Entities | ||
| { | ||
| /// <summary> | ||
| /// PimsDispositionFileProperty class, provides an entity for the datamodel to manage Disposition File's Sale. |
There was a problem hiding this comment.
nit: PimsDispositionFileProperty -> PimsDispositionSale
| namespace Pims.Dal.Entities | ||
| { | ||
| /// <summary> | ||
| /// PimsDspPurchAgent class, provides an entity for the datamodel to manage the relationship between the Disposition Sale and the Purchaser Agent. |
There was a problem hiding this comment.
nit: PimsDspPurchAgent -> PimsDspPurchSolicitor
