bito-code-review[bot] commented on code in PR #41346:
URL: https://github.com/apache/superset/pull/41346#discussion_r3462569840
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/buildQuery.test.ts:
##########
@@ -1363,4 +1365,170 @@ describe('plugin-chart-ag-grid-table', () => {
expect(query.extras?.where || undefined).toBeUndefined();
});
});
+
+ describe('buildQuery - server pagination row limit', () => {
+ const serverPaginationFormData: TableChartFormData = {
+ ...basicFormData,
+ server_pagination: true,
+ };
+
+ test('uses user row limit when it is lower than server page size', () => {
+ const { queries } = buildQuery(
+ {
+ ...serverPaginationFormData,
+ row_limit: 10,
+ server_page_length: 20,
+ slice_id: 101,
+ },
+ {
+ ownState: {
+ currentPage: 0,
+ pageSize: 20,
+ },
+ },
+ );
+
+ expect(queries[0]).toMatchObject({
+ row_limit: 10,
+ row_offset: 0,
+ });
+ });
+
+ test('limits server page size by remaining rows inside user row limit', ()
=> {
+ const { queries } = buildQuery(
+ {
+ ...serverPaginationFormData,
+ row_limit: 120,
+ server_page_length: 50,
+ slice_id: 102,
+ },
+ {
+ ownState: {
+ currentPage: 2,
+ pageSize: 50,
+ },
+ },
+ );
+
+ expect(queries[0]).toMatchObject({
+ row_limit: 20,
+ row_offset: 100,
+ });
+ });
+
+ test('clamps pages beyond the row limit instead of emitting row_limit: 0',
() => {
+ const { queries } = buildQuery(
+ {
+ ...serverPaginationFormData,
+ row_limit: 120,
+ server_page_length: 50,
+ slice_id: 103,
+ },
+ {
+ ownState: {
+ // Page 5 is well past the cap; offset would be 250 > 120, which
+ // previously made row_limit collapse to 0 ("no limit").
+ currentPage: 5,
+ pageSize: 50,
+ },
+ },
+ );
+
+ expect(queries[0].row_limit).not.toBe(0);
+ expect(queries[0]).toMatchObject({
+ row_limit: 20,
+ row_offset: 100,
+ });
+ });
+
+ test('restores the full first-page row limit after a filter change reset',
() => {
+ // Uncached export lets us seed cachedChanges directly; the default
+ // export overrides extras with its own closure.
+ const { queries } = buildQueryUncached(
+ {
+ ...serverPaginationFormData,
+ row_limit: 120,
+ server_page_length: 50,
+ slice_id: 104,
+ },
+ {
+ // User was on the capped last page (row_limit would be 20)...
+ ownState: {
+ currentPage: 2,
+ pageSize: 50,
+ },
+ // ...then an external filter changed, so the cached filters differ
+ // from the current ones and pagination resets to page 0.
+ extras: {
+ cachedChanges: {
+ 104: [{ col: 'state', op: '==', val: 'previous' }],
+ },
+ },
+ },
+ );
+
+ expect(queries[0].row_limit).not.toBe(0);
+ expect(queries[0]).toMatchObject({
+ row_limit: 50,
+ row_offset: 0,
+ });
+ });
+
+ test('persists the user page size, not the capped limit, on filter reset',
() => {
+ const setDataMask = jest.fn();
+ buildQueryUncached(
+ {
+ ...serverPaginationFormData,
+ row_limit: 120,
+ server_page_length: 50,
+ slice_id: 106,
+ },
+ {
+ // On the capped last page, the per-request row_limit is 20.
+ ownState: {
+ currentPage: 2,
+ pageSize: 50,
+ },
+ extras: {
+ cachedChanges: {
+ 106: [{ col: 'state', op: '==', val: 'previous' }],
+ },
+ },
+ hooks: { setDataMask, setCachedChanges: jest.fn() },
+ },
+ );
+
+ // The persisted page size must stay 50, not collapse to the capped 20.
+ expect(setDataMask).toHaveBeenCalledWith(
+ expect.objectContaining({
+ ownState: expect.objectContaining({
+ currentPage: 0,
+ pageSize: 50,
+ }),
+ }),
+ );
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to assert the `setCachedChanges` mock is
technically correct as it is called unconditionally in the implementation.
However, your assessment that asserting it here would distract from the test's
primary focus on page-size persistence is valid. You may choose to either add
the assertion to satisfy the reviewer or document in the test why it is
intentionally omitted to maintain focus on the specific behavior being verified.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -58,7 +58,7 @@ export function getQueryMode(formData: TableChartFormData) {
return hasRawColumns ? QueryMode.Raw : QueryMode.Aggregate;
}
-const buildQuery: BuildQuery<TableChartFormData> = (
+export const buildQuery: BuildQuery<TableChartFormData> = (
Review Comment:
<!-- Bito Reply -->
The user's decision to defer the consolidation of `buildQuery` logic to a
separate pull request is reasonable. Given that this refactor involves multiple
plugins and is outside the scope of the current row-limit fix, addressing it
independently will help maintain a focused and manageable PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
