[GabrielCWT]

• Implement pagination through limit and offset
• Refactor code in assessments to use ecto querying instead of raw sql
• Implement filtering using query parameters
• Change response for AdminGradingController :index to return an object with "count" and "data" instead of an array of submissions. Where "count" is the total number of submissions for the query (For pagination) and "data" is the array of submissions.
• Update assessment tests to account for change in response.
• Update Notification/Email tests to account for change in response

Resolves #926

[GabrielCWT]

TODO:

• Refactor code
• Update tests to account for change in response
• Format once all refactoring is done

[coveralls]

coverage: 94.981% (-0.3%) from 95.316%
when pulling fa2cb80 on GabrielCWT:gabriel/pagination
into dd89ec9 on source-academy:master.

[josh1248]

(minor change) hi gabriel, i think the filter now to restrict views runs only when group == true, but this value represents showAllGroups. Hence, the intended functionality is actually flipped. Could you help to update this to false? Apologies for the late comment. Thank you! (Assessments.ex 1430)

[GabrielCWT]

(minor change) hi gabriel, i think the filter now to restrict views runs only when group == true, but this value represents showAllGroups. Hence, the intended functionality is actually flipped. Could you help to update this to false? Apologies for the late comment. Thank you! (Assessments.ex 1430)

@josh1248 To my knowledge it is meant to be whether "only the groups under the grader should be returned". See the old function all_submissions_by_grader_for_index in line 1244. There is a doc which explains it

[josh1248]

okies, updated to match!

[RichDom2185]

Relevant frontend PR: source-academy/frontend#2787

[RichDom2185]

Looks good! The builder pattern makes it really clean haha.

Are we going to do away with the old routes? What was the additional controller for?

[GabrielCWT]

@RichDom2185 I can remove the old stuff in assessments.ex and admin_grading_controller.ex if everything looks good. With regards to the routes, I'm not too sure which old routes you are talking about since we are using the same route as before.

[RichDom2185]

@RichDom2185 I can remove the old stuff in assessments.ex and admin_grading_controller.ex if everything looks good. With regards to the routes, I'm not too sure which old routes you are talking about since we are using the same route as before.

Sorry, by "routes" I meant the old stuff you mentioned. But yes, I think it's fine to remove, since we're aiming for a breaking change, not a safe migration (in which case we would have both old and new functions first, then migrate frontend to new, then delete old entirely – that way there would be no downtime).

[GabrielCWT]

@RichDom2185 I can remove the old stuff in assessments.ex and admin_grading_controller.ex if everything looks good. With regards to the routes, I'm not too sure which old routes you are talking about since we are using the same route as before.

Sorry, by "routes" I meant the old stuff you mentioned. But yes, I think it's fine to remove, since we're aiming for a breaking change, not a safe migration (in which case we would have both old and new functions first, then migrate frontend to new, then delete old entirely – that way there would be no downtime).

@RichDom2185 There is still the ungraded_only param which is unresolved, not too sure what it does so I will leave the TODO comment on top of the new function.

[GabrielCWT]

Resolves #926

[GabrielCWT]

@RichDom2185 could you look through 2e3be04, I wasn't sure if notGradedFully should fall under the submissions builder

[RichDom2185]

@RichDom2185 could you look through 2e3be04, I wasn't sure if notGradedFully should fall under the submissions builder

Looks good!

[RichDom2185]

LGTM, thanks a lot!