psp-7598 | Properties list view filter by ownership #3781
Conversation
FuriousLlama
commented
Feb 9, 2024
FuriousLlama
commented
|
✅ No secrets were detected in the code. |
1 similar comment
|
✅ No secrets were detected in the code. |
|
@FuriousLlama backend build failing, frontend tests failing. |
There was a problem hiding this comment.
I'm a little nervous about these map deletions since mapping errors only show up at runtime. Since this is a release sprint, are you 100% confident these deletions are safe?
There was a problem hiding this comment.
Yes, given that the model it was mapping was deleted I have confidence it was not being used anywhere else, and therefore, not mapped
| { | ||
| if (tempSort[i].StartsWith("Location")) | ||
| { | ||
| tempSort[i] = tempSort[i].Replace("Location", "Address.MunicipalityName"); |
There was a problem hiding this comment.
I can't check the story, but are we sure location maps to municipality (I know we also have a general location field in the db)
There was a problem hiding this comment.
yes, that is mapped on the frontend columns.
| { | ||
| // The order will affect the display in the frontend. For now in alphabetical order. | ||
| // i.e. [Core Inventory, Disposed, Other Interest, Property of Interest] | ||
| var direction = this.Sort[i].Split(' ')[1]; |
There was a problem hiding this comment.
is this safe (ie. we always have a direction of asc or desc)?
There was a problem hiding this comment.
yes, the sort expects a direction to be able to be process later
| @@ -2,6 +2,7 @@ | |||
| using System.Linq; | |||
| using System.Linq.Expressions; | |||
| using System.Reflection; | |||
| using DocumentFormat.OpenXml.Office2010.Excel; | |||
There was a problem hiding this comment.
I get this sometimes too, but I'm pretty sure this is not needed.
There was a problem hiding this comment.
weird, i've cleansed up other instances of it.
| if (orderExpression != null) | ||
| { | ||
| var genericMethod = OrderByMethod.MakeGenericMethod(typeof(T), rType); | ||
| var genericMethod = orderMethod.MakeGenericMethod(typeof(T), rType); |
There was a problem hiding this comment.
oh right, I forgot about this generic sort implementation we have.
| @@ -27,4 +29,5 @@ export const defaultPropertyFilter: IPropertyFilter = { | |||
| longitude: '', | |||
| page: undefined, | |||
| quantity: undefined, | |||
| ownership: 'isCoreInventory,isPropertyOfInterest,isOtherInterest', | |||
There was a problem hiding this comment.
Not part of the default set, per the story definition.
|
|
||
| const [pageSize, setPageSize] = useState(10); | ||
| const [pageIndex, setPageIndex] = useState(0); | ||
| const [pageCount, setPageCount] = useState(0); | ||
| const [totalItems, setTotalItems] = useState(0); | ||
| const [sort, setSort] = useState<TableSort<IProperty>>({}); | ||
| const [sort, setSort] = useState<TableSort<any>>({}); |
There was a problem hiding this comment.
are they <any> 's in this file intentional?
There was a problem hiding this comment.
yeah I was matching the other definitions. I made it Api_Property instead
|
Finished code review, but will hold off on approval until I can actually run the code. |
|
✅ No secrets were detected in the code. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #3781 +/- ##
==========================================
- Coverage 75.75% 75.69% -0.06%
==========================================
Files 1448 1446 -2
Lines 38076 38080 +4
Branches 7136 7154 +18
==========================================
- Hits 28844 28826 -18
- Misses 8949 8967 +18
- Partials 283 287 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
✅ No secrets were detected in the code. |
|
✅ No secrets were detected in the code. |
1 similar comment
|
✅ No secrets were detected in the code. |
