Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transfer pagination work from frontend to backend #2787

Merged
merged 110 commits into from
Feb 23, 2024

Conversation

josh1248
Copy link
Contributor

@josh1248 josh1248 commented Feb 17, 2024

Paired with backend PR: source-academy/backend#1065.

Description

This request shifts work of filtering from the frontend to the backend. Page state is converted into SQL offsets and limit for the backend, which stop querying once enough entries to fill the page is reached, hence saving time. State management by the grading submissions table state has been greatly reduced. Pagination work by tanstack has been removed.

  • Pagination work by tanstack has been removed, and now relies on the page and pageSize state supplied by the main page. The backend payload contains the total entries, which is converted to total pages. Offset in SQL querying is also converted within this page.
  • Filtering for all categories have been changed to the backend. Adding or removing these filters / dropdown options resets the page index and requests a fresh set of entries to be displayed.
  • Grading status dropdown option filter has been migrated to the backend. To simplify logic, the individual Grading status within the page is no longer filterable.

Further polish to explore to extend this feature change:

  • Frontend-backend query conversion is currently hard-coded for GradingSubmissionTable to reverse the backend-frontend conversion by RequestsSaga. These conversion functions could be coupled.
  • Smart caching of results from backend. For example, if the user visits page X, do LIMIT X * pageSize, store all results in frontend.
  • Re-establish export grading CSV.

Type of change

This is a breaking change:

  • It expects a different backend response, {count: number, data: array of submissions} from backend PR number 1065, differing from current response structure, which is a simple array of submissions.
  • It breaks the export grading CSV function, which can only export off the limited backend queries it gets.
  • It will diminish the functionality of the Grading Summary page as it can only read off the limited backend queries it gets. (Edit: no longer an issue as the grading summary page will be deleted

Feature removals:

  • Individual grading status can no longer be clicked on for filtering.
  • Search bar feature has been restricted from global filter to assessment name to avoid OR or comparative querying.

Feature enhancement:

  • additional dropdown option to allow the user to set the number of entries per page that they see.
  • Introduces 2 additional page control buttons "<<" and ">>" that set the page to the minimum and maximum respectively.
image

…nsTable component to allow for page reset upon application of filter
Copy link
Contributor Author

@josh1248 josh1248 Feb 23, 2024

Choose a reason for hiding this comment

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

Previous behaviour:
if incompatible submissions filtering and ungraded only dropdown options were chosen, the submissions filtering option would override the dropdown. For example, given Filter status == "Attempting" and ungraded only dropdown value, the ungraded dropdown value is ignored, even if the expectations are 0 entries (because ungraded and status Attempting are mutually exclusive)

Current behaviour: dropdown options overrule filtering by progress status instead.

BUG: if mutually incompatible status + dropdown is introduced, 0 entries are shown to the user, but we still fetch by "Attempted" only. Our dropdown options now do frontend filtering!!! This must have something to do with Filters being stored as part of the workspace session (which you can verify if you select filters, go to another page, then go back). This is part of a larger bug (I will link here later to some bug report and enhancement i will be filing).

This bug does not occur for non-mutually exclusive intersections that play well together, like status==='Attempted' and viewing 'All': the backend still takes charge.

Time-intensive fix: Update logic so that Filters are no longer part of a Workspace state, which should kill off any remaining frontend filtering artifacts that this PR may not have removed.

Other fix possible: remove filter based on state of dropdown. setting 'viewing' to ungraded should remove progress filter (since viewing by ungraded implicitly requires all entries to have progress submitted.)

Fast fix: make progress non-Filterable.

image

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for catching this! I think it's an edge case, and I agree the filters should have not been stored in the state anyway (this was an oversight that was missed in the original PR). Regardless, it works for a normal use case so I think we can just leave it as a separate issue – this PR is getting large anyway.

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for helping to review my changes @josh1248 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants