1366 filtering on some pages broken#1371
Conversation
…er on it as a simple integer where required.
…Name property rather than custom filtering based on pattern matching and Linq.
…nager filtering. Also adjusted filtering by CurrentGrade to use a calculated integer value.
…te rather than being set in the calling code.
… been moved into the entity.
Adjusted Faculty and School filtering to use the Code for each as the filterable property. Removed filter from some columns where it didn't make much sense (costs, day rate, dates etc.)
PhilBradbury
requested review from
aharwood2,
gdonval,
philiptharrison and
sjmf
as code owners
aharwood2
left a comment
aharwood2
left a comment
There was a problem hiding this comment.
I think this probably needs a walkthrough so I can see the differences more clearly. Will need to check that the changes don't alter the UX in a way they might not like.
|
Walkthrough video now available at: https://www.loom.com/share/f3190d9bb90a4faeab6a5f51ef56f16e The egregious bracket after the value in one of the monetary columns has been removed. |
… to have a property to hold FundsReceived data to be able to filter on that.
…com/UoMResearchIT/CapX into 1366-filtering-on-some-pages-broken
Very nice @PhilBradbury - thanks much! |
| /// </summary> | ||
| /// <returns></returns> | ||
| [NotMapped] | ||
| public double FundsReceived { get; set; } |
There was a problem hiding this comment.
This makes me nervous as we have a property on a model that is invalid most of the time and is only populated correctly in a very specific context.
There was a problem hiding this comment.
I agree that it doesn't feel like the best way to do it, but it's either this or injecting a service to be able to to a getter that retrieves the calculated figure.. and then we're making it less decoupled (more coupled?). Neither option is great. :(
| FilterMode="FilterMode.SimpleWithMenu" | ||
| FilterOperator="FilterOperator.GreaterThanOrEquals"> | ||
| <Template Context="p"> | ||
| @(PaymentService.GetFundsReceived(Context, p.ProjectId).ToString("C0")) |
There was a problem hiding this comment.
I'm not sure on how "clean" putting a property on a model is where that value is only populated depending on where the model is used. Changing the approach from using LoadData to using built-in filtering means we do lose the flexibility the LoadData approach offers and have to do questionable design things like this. In some sense it is important to fix this feature but I'm not convinced this approach is the "correct" way to do it.
There was a problem hiding this comment.
We could remove the actual property and maintain a dictionary on the page (mapping the ProjectId to the calculated funds amount) and reference that instead? This would remove it from the Project class, but feels even dirtier!
There was a problem hiding this comment.
Pull request overview
This PR addresses the broken/crashing filtering behavior on the People and Manage Projects pages (issue #1366), likely triggered by changes in Radzen’s filtering behavior, by shifting filtering/sorting back toward Radzen’s built-in mechanisms and introducing grid-friendly derived properties.
Changes:
- Adjust Radzen DataGrid filtering mode/config and align column
FilterProperty/SortPropertywith actual nested fields (e.g.,School.Faculty.Code,LineManager.Name). - Introduce derived, non-persisted properties to support filtering in grids (
Project.FundsReceived,Person.CurrentGrade). - Remove/reshape custom filtering logic in the page code-behind to reduce filter crashes and improve paging/counting behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| PPMTool/Pages/Projects.razor.cs | Populates FundsReceived for grid filtering; removes custom Faculty/School filtering block. |
| PPMTool/Pages/Projects.razor | Updates grid/column filter modes and adds typed/date/numeric filter configuration; switches to filtering on nested School/Faculty properties. |
| PPMTool/Pages/People.razor.cs | Simplifies custom filtering to SkillTags only; relies on Radzen’s args.Filter for other filters; updates sorting/paging/count flow. |
| PPMTool/Pages/People.razor | Updates Line Manager column to filter/sort by LineManager.Name; disables filtering on problematic derived columns; switches grade display to CurrentGrade. |
| PPMTool.Data/Entities/Project.cs | Adds [NotMapped] FundsReceived to support grid filtering. |
| PPMTool.Data/Entities/Person.cs | Adds [NotMapped] CurrentGrade to support grid filtering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add details of FundsReceived so we can use it for filtering in the grid as a property | ||
| foreach (Project p in query) | ||
| { | ||
| p.FundsReceived = PaymentService.GetFundsReceived(Context, p.ProjectId); | ||
| } |
| // ---- BUILT-IN RADZEN FILTERING ---- | ||
| if (!string.IsNullOrWhiteSpace(args.Filter)) | ||
| { | ||
| // Check details of the sort | ||
| if (args.Sorts is { } sorts && sorts.Any()) | ||
| { | ||
| var sort = args.Sorts?.First(); | ||
| if (sort.Property == "SkillsCount") | ||
| { | ||
| // Apply the ordering process on skills count manually | ||
| if (sort.SortOrder == SortOrder.Ascending) | ||
| { | ||
| query = query.OrderBy(x => TagService.GetCountForPerson(Context, x.PersonId)); | ||
| } | ||
| else | ||
| { | ||
| query = query.OrderByDescending(x => TagService.GetCountForPerson(Context, x.PersonId)); | ||
| } | ||
| } | ||
| else if (sort.Property == "LineManager") | ||
| { | ||
| query = sort.SortOrder == SortOrder.Ascending ? | ||
| query.OrderBy(x => x.LineManager != null ? x.LineManager.Name : "") : | ||
| query.OrderByDescending(x => x.LineManager != null ? x.LineManager.Name : ""); | ||
| } | ||
|
|
||
| // No special handling required | ||
| else | ||
| { | ||
| // Sort via the OrderBy method | ||
| query = query.OrderBy(args.OrderBy); | ||
| } | ||
| } | ||
| query = query.Where(args.Filter); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Closes #1366