Fix issue with assignments without grading period not being shown on Assignment List #2969

ndrsszsz · 2024-11-14T18:21:38Z

refs: MBL-18023
affects: Student, Teacher
release note: Fixed an issue where assignments that don't belong to any of the available grading periods were not shown at Assignments.
test plan: Log in with an account that has multiple grading periods and assignments that don't belong to any of those grading periods, and check Assignment List if they are showing all of the assignments properly.

Details

Updated AssignmentListViewModel to have the currently active grading period as the default one.
Fixed GetAssignmentsByGroup UseCase (all credit goes to Attila on this one).
Modified AssignmentListViewModel to get all assignment groups (independently from the grading periods) and apply the grading period filtering on them after. To achieve this I needed to append the stored properties of a Submission entity with grading_period_id which we always get from the API (but we didn't use it until now).

Test

Attila's account is a good one for this. File Upload 4 Copy is one of the assignments that weren't shown previously. You can see it in the screenshots.
See ticket for account info.

Screenshots

Assignment List

Before	After

Checklist

inst-danger · 2024-11-14T18:26:51Z

	Fails
🚫	Build failed, skipping coverage check

Release Note:

Fixed an issue where assignments that don't belong to any of the available grading periods were not shown at Assignments.

Affected Apps: Student, Teacher

MBL-18023

❌ XCTest failed: CoreTests/CourseSyncGradesInteractorLiveTests/testSuccessfulSync

failed - Unexpected failure while waiting on finish event: Error Domain=com.instructure Code=0 "No response or error received." UserInfo={NSLocalizedDescription=No response or error received.}

❌ XCTest failed: CoreTests/GetAssignmentsByGroupTests/testFetchesAssignmentsForEachGradingPeriod

XCTAssertEqual failed: ("0") is not equal to ("2")
XCTAssertEqual failed: throwing "NSRangeException: *** -[__NSArray0 objectAtIndex:]: index 0 beyond bounds for empty array"
failed

❌ XCTest failed: CoreTests/GetAssignmentsByGroupTests/testFetchesAssignmentsForEachGradingPeriodAndReturnsAssignmentsForAGradingPeriod

XCTAssertEqual failed: ("0") is not equal to ("1")
XCTAssertEqual failed: throwing "NSRangeException: *** -[__NSArray0 objectAtIndex:]: index 0 beyond bounds for empty array"
failed

❌ XCTest failed: CoreTests/GetAssignmentsByGroupTests/testFetchesAssignmentsForEachGradingPeriodWhenHideGradeIsFalse

XCTAssertEqual failed: ("0") is not equal to ("1")

Generated by 🚫 dangerJS against 300de1f

inst-danger · 2024-11-14T18:29:17Z

Teacher Build QR Code:

inst-danger · 2024-11-14T18:30:29Z

Student Build QR Code:

affects: Student, Teacher release note: Fixed an issue where assignments that don't belong to any of the available grading periods were not shown at Assignments. test plan: See ticket.

suhaibabsi-inst

Code +1

Core/Core/Assignments/AssignmentList/ViewModel/AssignmentListViewModel.swift

suhaibabsi-inst · 2024-11-18T12:46:36Z

For the intended change, it seems working good. But current period selection is always set to a specific value (here it is Spring) on screen appearance. So, if you change the viewed period to different one, and went into seeing an assignment details, when backing to the assignments list screen you'd always reset to that default period. Which doesn't seem intuitive.

See record below:

current_period_selection_reset_issue.mp4

ndrsszsz · 2024-11-18T16:51:58Z

@suhaibabsi-inst I have addressed your suggestions. Please check again.

rh12

Code review + 1 (minus the tests that need updating)

rh12 · 2024-11-18T22:38:13Z

Core/Core/Assignments/AssignmentList/ViewModel/AssignmentListViewModel.swift

+        if self?.selectedGradingPeriod == nil {
+            self?.defaultGradingPeriod = self?.currentGradingPeriod
+            self?.selectedGradingPeriod = self?.defaultGradingPeriod


I would add a guard let self else { return } before all this nil comparisons and assignments, even it is not strictly needed.

rh12 · 2024-11-18T22:53:36Z

Core/Core/Assignments/AssignmentList/ViewModel/AssignmentListViewModel.swift

+        isFilterIconSolid = (1 ..< AssignmentFilterOption.allCases.count).contains(selectedFilterOptions.count)
+            || selectedGradingPeriod != defaultGradingPeriod


This logic is different then the one in filterOptionsDidUpdate().
Is it intentional? Why is this logic needed here if the value is set just before it (at the end of that method)

Regardless I would extract two pieces of logic, because they are used in multiple places:

private var isFilteringCustom: Bool { // all filters selected is the same as no filter selected selectedFilterOptions.isNotEmpty && selectedFilterOptions.count != AssignmentFilterOption.allCases.count } private func updateFilterIcon() { isFilterIconSolid = ... }

rh12 · 2024-11-18T23:00:30Z

Core/Core/Assignments/AssignmentList/ViewModel/AssignmentListViewModel.swift

-                    }
+            assignmentGroups.forEach { group in
+                let groupAssignments: [Assignment] = assignments.filter { $0.assignmentGroup == group }
+                if !groupAssignments.isEmpty {


Suggested change

if !groupAssignments.isEmpty {

guard groupAssignments.isNotEmpty else { return }

rh12 · 2024-11-18T23:02:34Z

Core/Core/Assignments/AssignmentList/ViewModel/AssignmentListViewModel.swift

@@ -189,6 +194,12 @@ public class AssignmentListViewModel: ObservableObject {
    }

    private func filterAssignments(_ assignments: [Assignment]) -> [Assignment] {
+        // Filtering by grading period except if all is selected (nil)


Suggested change

// Filtering by grading period except if all is selected (nil)

// Filtering by grading period except if "All" is selected (nil)

to clarify (without needing to check elsewhere) that All is an option, not all of the options

ndrsszsz added 2 commits November 14, 2024 13:32

commit message

f09e2fa

fix - assignments without a grading period missing from assignment list

5b77398

ndrsszsz self-assigned this Nov 14, 2024

ndrsszsz requested review from vargaat, szabinst, rh12 and suhaibabsi-inst as code owners November 14, 2024 18:21

refs: MBL-18023

0e01ffc

affects: Student, Teacher release note: Fixed an issue where assignments that don't belong to any of the available grading periods were not shown at Assignments. test plan: See ticket.

suhaibabsi-inst reviewed Nov 18, 2024

View reviewed changes

Core/Core/Assignments/AssignmentList/ViewModel/AssignmentListViewModel.swift Outdated Show resolved Hide resolved

requested changes

eecec8a

ndrsszsz requested a review from suhaibabsi-inst November 18, 2024 16:52

fix - testAssignmentsPopulate

300de1f

rh12 reviewed Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with assignments without grading period not being shown on Assignment List #2969

Fix issue with assignments without grading period not being shown on Assignment List #2969

ndrsszsz commented Nov 14, 2024 •

edited

Loading

inst-danger commented Nov 14, 2024 •

edited by jira bot

Loading

inst-danger commented Nov 14, 2024 •

edited

Loading

inst-danger commented Nov 14, 2024 •

edited

Loading

suhaibabsi-inst left a comment

suhaibabsi-inst commented Nov 18, 2024 •

edited

Loading

ndrsszsz commented Nov 18, 2024

rh12 left a comment

rh12 Nov 18, 2024

rh12 Nov 18, 2024

rh12 Nov 18, 2024

rh12 Nov 18, 2024

		isFilterIconSolid = (1 ..< AssignmentFilterOption.allCases.count).contains(selectedFilterOptions.count)
		\|\| selectedGradingPeriod != defaultGradingPeriod

	if !groupAssignments.isEmpty {
	guard groupAssignments.isNotEmpty else { return }

	// Filtering by grading period except if all is selected (nil)
	// Filtering by grading period except if "All" is selected (nil)

Fix issue with assignments without grading period not being shown on Assignment List #2969

Are you sure you want to change the base?

Fix issue with assignments without grading period not being shown on Assignment List #2969

Conversation

ndrsszsz commented Nov 14, 2024 • edited Loading

Details

Test

Screenshots

Assignment List

Checklist

inst-danger commented Nov 14, 2024 • edited by jira bot Loading

Release Note:

Affected Apps: Student, Teacher

inst-danger commented Nov 14, 2024 • edited Loading

inst-danger commented Nov 14, 2024 • edited Loading

suhaibabsi-inst left a comment

Choose a reason for hiding this comment

suhaibabsi-inst commented Nov 18, 2024 • edited Loading

ndrsszsz commented Nov 18, 2024

rh12 left a comment

Choose a reason for hiding this comment

rh12 Nov 18, 2024

Choose a reason for hiding this comment

rh12 Nov 18, 2024

Choose a reason for hiding this comment

rh12 Nov 18, 2024

Choose a reason for hiding this comment

rh12 Nov 18, 2024

Choose a reason for hiding this comment

ndrsszsz commented Nov 14, 2024 •

edited

Loading

inst-danger commented Nov 14, 2024 •

edited by jira bot

Loading

inst-danger commented Nov 14, 2024 •

edited

Loading

inst-danger commented Nov 14, 2024 •

edited

Loading

suhaibabsi-inst commented Nov 18, 2024 •

edited

Loading