Skip to content

SC-399 - paginate ByUser page. Includes fix for race condition#854

Open
nbeatty-gpa wants to merge 15 commits intomasterfrom
SC-115-Add-pagination-to-all-tables-that-do-not-have-it
Open

SC-399 - paginate ByUser page. Includes fix for race condition#854
nbeatty-gpa wants to merge 15 commits intomasterfrom
SC-115-Add-pagination-to-all-tables-that-do-not-have-it

Conversation

@nbeatty-gpa
Copy link
Copy Markdown
Contributor

@nbeatty-gpa nbeatty-gpa commented May 6, 2026

paginate user account page, and guard against race conditions that could cause negative pages.

SC-399

@elwills elwills requested a review from prestoncraw May 6, 2026 17:04
Comment on lines +184 to +207
IEnumerable<DataRow> filteredRows = dataTable.AsEnumerable();
IEnumerable<SQLSearchFilter> searchesToApply = postData.Searches.Where(flt => !IsInDatabase(flt.FieldName));
foreach (SQLSearchFilter search in searchesToApply)
{
string wildcardPattern = Regex.Escape(search.SearchText.ToLower()).Replace(@"\*", ".*");
switch (search.Operator)
{
case "=":
filteredRows = filteredRows.Where((row) => row.Field<string>(search.FieldName).ToLower() == search.SearchText.ToLower());
break;
case "LIKE":
filteredRows = filteredRows.Where((row) => Regex.IsMatch(row.Field<string>(search.FieldName).ToLower(), wildcardPattern));
break;
case "NOT LIKE":
filteredRows = filteredRows.Where((row) => !Regex.IsMatch(row.Field<string>(search.FieldName).ToLower(), wildcardPattern));
break;
case "IN":
List<string> groupTypes = search.SearchText.Trim('(', ')').Split(',').ToList();
filteredRows = filteredRows.Where((row) => groupTypes.Contains(row.Field<string>(search.FieldName)));
break;
default:
throw new Exception("Operator not found for Filter.");
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would create a static method that both SecurityGroup/UserAccount can use if they are the exact same logic.

Comment on lines 112 to 115
React.useEffect(() => {
if (currentPage >= totalPages) { }
if (currentPage >= totalPages && totalPages > 0)
setPage(totalPages)
}, [totalPages])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems as this logic is repeated multiple times in different components with varying logic in them. At the very least they should be sharing the same logic if they are trying to do the same thing. Even better would be to create a reusable hook.

For example check -- Source/Applications/SystemCenter/wwwroot/Scripts/TSX/SystemCenter/Settings/Setting.tsx

It has different logic than whats above but seem to be attempting to do the same thing.

Comment on lines +131 to +136
React.useEffect(() => {
if (currentPage >= totalPages)
setPage(totalPages)
else
setPage(1)
}, [totalPages])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above comment, same intended logic but different code. Would benefit from a reusable hook also missing a dep

if (searchStatus == 'uninitiated' || searchStatus == 'changed')
dispatch(SecurityGroupSlice.DBSearch({ sortField, ascending, filter: search }))
dispatch(SecurityGroupSlice.PagedSearch({ sortField, ascending, filter: search, page: currentPage - 1 }))
}, [searchStatus])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing multiple deps here

setPage(totalPages)
else
setPage(1)
}, [totalPages])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same comment as above, as well as this is missing currentPage dep.

React.useEffect(() => {
if (currentPage >= totalPages && totalPages > 0)
setPage(totalPages)
}, [totalPages])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ssame as above comment also missing deps

Comment on lines +92 to +95
if (currentPage >= totalPages && totalPages > 0)
setPage(totalPages)
}
else
setPage(0)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above, and missing dep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants