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

Update GraphQL Queries with Pagination #3064

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

suhaibabsi-inst
Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst commented Jan 14, 2025

refs: MBL-18026
affects: Student, Teacher
release note: Update GraphQL Queries with Pagination.

What's Changed

All GraphQL queries used to fetch a list of items were modified to include paging, and those are:

1- Teacher's Comments Library for Assignment Submission.

Request type: APICommentLibraryRequest
Used in Teacher app.

2- Assignment Picker List of Submit Assignment Extension.

Request type: AssignmentPickerListRequest
Used in "SubmitAssignment" extension of Student app.

3- Settings page course sections list of Assignment Submissions List.

GetAssignmentPostPolicyInfoRequest type was splitted into two separate requests:
A. GetPostPolicyCourseSectionsRequest: was modified to include paging.
B. GetPostPolicyAssignmentSubmissionsRequest : paging was not included, because it was used to calculate counts rather than presenting information of submissions.

Used in Teacher app.

Paging Logic

New main types were introduced to encapsulate paging logic for both UIKit and SwiftUI contexts:
PagingButton - SwiftUI View to be added to the very bottom of the list.

In UIKit context, to handle paging in UITableView, you need to consider the following ones.
Paging<C> - A presenter object used to handle paging logic in UITableView instances, you need to create one per a table view.
PageLoadingCell - A cell that can be dequeued in UITableView with loading & call-to-action variations, to be shown based on the logic encapsulated in the Paging instance associated with.

Test Plan

1 - Teacher's Comments Library:

  • Make sure to enable comments suggestions for SpeedGrader. See this page.
  • Open Teacher app, then navigate to a student submission made to any assignment.
  • Open comments sheet, the tap on text input field.
  • Comments library would show up to choose from. That list should be loading comments in pages.

2 - Assignment Picker List of Submit Assignment Extension.

  • Go to Files or Photos app, then user "Share" option on any photo or valid file for assignment submission.
  • Choose Student app, then select the course you are interested in using "Select course" field of the shown form.
  • Then tap on "Select Assignment" field to show list of assignment to pick from.
  • That list should be loading assignments in pages.

3- Settings page course sections list of Assignment Submissions List.

  • In Teacher app, go to any assignment submissions list page.
  • Tap on the top right button with the "eye" icon.
  • Turn on, "Specific Sections" toggle on.
  • Course sections must be loaded below in pages, this for both of the screen tabs (Post Marks / Hide Marks)

Note:

"List loading in pages" means that you get to notice loading indicator when you scroll to the very bottom of it, if loading failed "Load More" button is shown to allow user to try fetching again.

Video Recordings

1- Comments Library

case1_comments_library.mp4

2- Assignment Picker List

case2_post_policy.mp4

3- Post Policy Settings Page

case2_post_policy_edit0.mov

Checklist

  • Follow-up e2e test ticket created
  • A11y checked
  • Tested on phone
  • Tested on tablet
  • Tested in dark mode
  • Tested in light mode
  • Approve from product

@inst-danger
Copy link
Contributor

inst-danger commented Jan 14, 2025

Warnings
⚠️ One or more files are below the minimum test coverage 50%

Release Note:

Update GraphQL Queries with Pagination

Affected Apps: Student, Teacher

MBL-18026

Coverage New % Master % Delta
Canvas iOS 91.34% 91.52% -0.18%
Core/Core/Common/CommonUI/Presenter/PagingPresenter.swift 26.09% -- --

Generated by 🚫 dangerJS against c29c842

refs: MBL-18026
affects: Student, Teacher
release note: Update GraphQL Queries with Pagination
@inst-danger
Copy link
Contributor

inst-danger commented Jan 15, 2025

Teacher Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Jan 15, 2025

Student Build QR Code:

@suhaibabsi-inst suhaibabsi-inst marked this pull request as ready for review January 21, 2025 17:54
@suhaibabsi-inst suhaibabsi-inst self-assigned this Jan 27, 2025
@rh12
Copy link
Contributor

rh12 commented Jan 28, 2025

Could you please doublecheck that you cover all changes/additions with unit tests?
This would also ensure that the overall coverage is not reduced PR by PR.

Copy link
Collaborator

@vargaat vargaat left a comment

Choose a reason for hiding this comment

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

Nice work on getting the foundations ready for this! I think on the comment library and on the share extensions's assignment list we should use exhaust because there are logics working on local filtering / selecting from the list.

  1. On SpeedGrader's comment list there's a local filtering for the word entered into the comment library's text field so we should know all entries to be able to locally filter the list. At the moment on my account I have more than 20 items (page size is 20) and when I first enter a keyword the filtering finds it despite I didn't scroll to the 2nd page. I think this is some glitch that makes it automatically download the 2nd page. The filtering not working correctly is visible when you have something already entered into the field and you open the dialog a second time.
ScreenRecording_01-31-2025.11-19-40_1.MP4
  1. The share extensions's assignment list should pre-select the course and assignment if a file from the student app is being shared. A common scenario that a student opens a pdf from a course, annotates it inside the app and submits back to an assignment right away. In this scenario the extension won't find the assignment if it's not on the first page of the result list.

Core/Core/Common/CommonUI/Presenter/PagingPresenter.swift Outdated Show resolved Hide resolved
Core/Core/Common/CommonUI/Presenter/PagingPresenter.swift Outdated Show resolved Hide resolved
Core/Core/Common/CommonUI/Presenter/PagingPresenter.swift Outdated Show resolved Hide resolved
Core/Core/Common/CommonUI/SwiftUIViews/PagingButton.swift Outdated Show resolved Hide resolved
@suhaibabsi-inst
Copy link
Contributor Author

Nice work on getting the foundations ready for this! I think on the comment library and on the share extensions's assignment list we should use exhaust because there are logics working on local filtering / selecting from the list.

  1. On SpeedGrader's comment list there's a local filtering for the word entered into the comment library's text field so we should know all entries to be able to locally filter the list. At the moment on my account I have more than 20 items (page size is 20) and when I first enter a keyword the filtering finds it despite I didn't scroll to the 2nd page. I think this is some glitch that makes it automatically download the 2nd page. The filtering not working correctly is visible when you have something already entered into the field and you open the dialog a second time.

For lists that include filtering, I think in order to utilize paging the proper way, we need to make use of some "query" argument on the GraphQL functions to do that on server and remove logic of filtering locally.
For comments library, function commentBankItemsConnection has the argument query to serve that purpose. Perhaps, I am going to revisit that. Let me know your thoughts about it.

@suhaibabsi-inst
Copy link
Contributor Author

  1. The share extensions's assignment list should pre-select the course and assignment if a file from the student app is being shared. A common scenario that a student opens a pdf from a course, annotates it inside the app and submits back to an assignment right away. In this scenario the extension won't find the assignment if it's not on the first page of the result list.

I am not sure I understood this point, assignments list shown on share extension requires that assignments have submission type of "online_upload" enabled for them. Those having only "student_annotation" enabled for them, will get filtered out locally. Unfortunately, GraphQL function for course assignments doesn't have the option to filter against submission types. So, perhaps, we would need to get back to using exhaust for this in particular.

@vargaat
Copy link
Collaborator

vargaat commented Feb 3, 2025

For lists that include filtering, I think in order to utilize paging the proper way, we need to make use of some "query" argument on the GraphQL functions to do that on server and remove logic of filtering locally.
For comments library, function commentBankItemsConnection has the argument query to serve that purpose. Perhaps, I am going to revisit that. Let me know your thoughts about it.

Yes, that would be the ideal solution but if such query is not available we can still do an exhaust and filter locally. One comment library use-case is definitely broken with the current solution so we have to do something about it.

I am not sure I understood this point, assignments list shown on share extension requires that assignments have submission type of "online_upload" enabled for them. Those having only "student_annotation" enabled for them, will get filtered out locally. Unfortunately, GraphQL function for course assignments doesn't have the option to filter against submission types. So, perhaps, we would need to get back to using exhaust for this in particular.

Here a video of the automatic assignment selection feature that I think won't work if the assignment is on the second page.

ScreenRecording_02-03-2025.10-36-26_1.MP4

@suhaibabsi-inst
Copy link
Contributor Author

More unit tests are coming to increase the coverage.
Meanwhile, please feel free to leave your comment if you have any thought for improvement.

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.

4 participants