PSP-6829 : Management activity - list view #3526
Conversation
eddherrera
commented
Oct 19, 2023
eddherrera
commented
|
✅ No secrets were detected in the code. |
Codecov Report
@@ Coverage Diff @@
## dev #3526 +/- ##
==========================================
+ Coverage 70.62% 70.65% +0.02%
==========================================
Files 1338 1344 +6
Lines 32126 32306 +180
Branches 6006 6030 +24
==========================================
+ Hits 22690 22826 +136
- Misses 9189 9231 +42
- Partials 247 249 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| var propertyManagementActivity = _propertyRepository.GetManagementActivityById(managementActivityId); | ||
| if(propertyManagementActivity.PropertyId != propertyId) | ||
| { | ||
| throw new BadRequestException($"PropertyManagementActivity with Id: {managementActivityId} and PropertyId {propertyId} doesn't exists"); |
There was a problem hiding this comment.
nit: should be "doesn't exist" (with no s)
| var sucess = _propertyRepository.TryDeletePropertyActivity(managementActivityId); | ||
| _propertyRepository.CommitTransaction(); | ||
|
|
||
| return sucess; |
| @@ -5,6 +5,7 @@ | |||
| using System.Text.RegularExpressions; | |||
| using Microsoft.EntityFrameworkCore; | |||
| using Microsoft.Extensions.Logging; | |||
| using Microsoft.VisualBasic; | |||
There was a problem hiding this comment.
removed, no idea how it got there.
| @@ -464,6 +465,74 @@ public HashSet<long> GetMatchingIds(PropertyFilterCriteria filter) | |||
| return query.Select(p => p.PropertyId).ToHashSet(); | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Return a summary List of Management activities for a especific property. | |||
|
|
||
| return Context.PimsPropPropActivities | ||
| .AsNoTracking() | ||
| .Include(x => x.PimsPropertyActivity) |
There was a problem hiding this comment.
Do you need the includes on the code navigation properties as well?
There was a problem hiding this comment.
I needed it for the Service to check that the activity status is 'not started'.
| } | ||
|
|
||
| /// <summary> | ||
| /// TryDelete the Activity asscociated with the property and if no property associated to activity delete the activicy as well. |
|
|
||
| if (deletedEntity is not null) | ||
| { | ||
| if (Context.PimsPropPropActivities.Count(x => x.PimsPropertyActivityId == deletedEntity.PimsPropertyActivityId) > 1) |
There was a problem hiding this comment.
can you explain what you are trying to do here? it seems like you are manually implement a cascade here but I believe we already have a db cascade in place for PimsPropertyActivity and PimsPropPropActivity. This top if statement also appears to be only removing the many-to-many, that seems odd if the caller requested that an activity by deleted by an activity id, as the activity will still be in the system, just not the first many-to-many with a matching id.
There was a problem hiding this comment.
As discussed, updated method name and added comments for clarity.
| } | ||
|
|
||
| [Fact] | ||
| public void Delete_PropertyManagementActivity_BadRequest_Activity_STARTED() |
There was a problem hiding this comment.
nit: naming convention -> STARTED
| return ( | ||
| <Table<PropertyActivityRow> | ||
| name="PropertyManagementActivitiesTable" | ||
| manualSortBy={false} |
There was a problem hiding this comment.
I think this should probably be true since you are passing your own sort methods, but I will confirm.
There was a problem hiding this comment.
not sure if it should. As it is it works properly.
| }) => { | ||
| const [sort, setSort] = React.useState<TableSort<Api_PropertyManagementActivity>>({}); | ||
|
|
||
| const mapSortField = (sortField: string) => { |
There was a problem hiding this comment.
huh, was the display order not a viable option here?
There was a problem hiding this comment.
negative, display order is not a property on the activity. Such property was removed from the model.
|
|
||
| return ( | ||
| <Section isCollapsable initiallyExpanded header="Activities List"> | ||
| <LoadingBackdrop show={isLoading} /> |
There was a problem hiding this comment.
I don't think you need this? there should already be a spinner on the table right?
There was a problem hiding this comment.
I followed the patter as the Contact list, but you are correct. this was updated to pass the flag to the table.
|
✅ No secrets were detected in the code. |
|
✅ No secrets were detected in the code. |
