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

Fix changing subject weights #1810

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,27 @@ class SubjectSettingsPageController extends ChangeNotifier {
return;
}

if (subject.weightType == WeightType.perGrade) {
state = SubjectSettingsError(
'Subject "${subject.name}" (id: $subjectId) has its weightType set to ${WeightType.perGrade}. This is not supported (yet).');
return;
}

_subjectName = subject.name;
final finalGradeType = _getFinalGradeType(subject.finalGradeTypeId);
_finalGradeTypeDisplayName = _getFinalGradeTypeDisplayName(finalGradeType);
_finalGradeTypeIcon = _getFinalGradeTypeIcon(finalGradeType);
_selectableGradeTypes = gradesService.getPossibleGradeTypes();

// Todo: Explain this workaround
_weights = IMap();

if (subject.weightType == WeightType.inheritFromTerm) {
// We show the weights from the term, but we need to copy them into the
// subject if the user changes them.
_weights = _getTerm()!.gradeTypeWeightings;
_weights = _weights.addAll(_getTerm()!.gradeTypeWeightings);
} else {
_weights = subject.gradeTypeWeights;
_weights = _weights.addAll(subject.gradeTypeWeights);
}

state = SubjectSettingsLoaded(view);
Expand Down Expand Up @@ -116,8 +125,9 @@ class SubjectSettingsPageController extends ChangeNotifier {
// In the constructor, we already copied the weights from the term. But they
// were only copied into the state. Now we need to copy them into the
// database.
for (final gradeType in _weights.keys) {
subRef.changeFinalGradeType(gradeType);
// Todo: Use weights from term for clarity?
for (final entry in _weights.entries) {
subRef.changeGradeTypeWeight(entry.key, entry.value);
await waitForFirestoreWriteLimit();
}

Expand Down
4 changes: 4 additions & 0 deletions app/test/grades/grades_test_common.dart
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,7 @@ TestSubject subjectWith({
GradeTypeId? finalGradeType,
Design? design,
List<ConnectedCourse> connectedCourses = const [],
bool ignoreWeightTypeAssertion = false,
}) {
final idRes = id ?? SubjectId(randomAlpha(5));
final nameRes = name ?? idRes.value;
Expand All @@ -448,6 +449,7 @@ TestSubject subjectWith({
gradeTypeWeights: gradeTypeWeights,
finalGradeType: finalGradeType,
design: design ?? Design.random(szTestRandom),
ignoreWeightTypeAssertion: ignoreWeightTypeAssertion,
);
}

Expand All @@ -474,7 +476,9 @@ class TestSubject {
this.weightType,
this.weight,
this.finalGradeType,
bool ignoreWeightTypeAssertion = false,
}) : assert(() {
if (ignoreWeightTypeAssertion) return true;
// Help developers to not forget to set the weightType if
// gradeTypeWeights or grade weights are set. This is not a hard
// requirement by the logic, so if you need to do it anyways then you
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,270 @@
// Copyright (c) 2025 Sharezone UG (haftungsbeschränkt)
// Licensed under the EUPL-1.2-or-later.
//
// You may obtain a copy of the Licence at:
// https://joinup.ec.europa.eu/software/page/eupl
//
// SPDX-License-Identifier: EUPL-1.2

import 'package:flutter_test/flutter_test.dart';
import 'package:sharezone/grades/grades_service/grades_service.dart';
import 'package:sharezone/grades/pages/subject_settings_page/subject_settings_page_controller.dart';

import '../../grades_test_common.dart';

void main() {
group('$SubjectSettingsPageController', () {
const termId = TermId('my-term');
const subjectId = SubjectId('maths');

late GradesTestController testController;
late TermSubjectRef subRef;

setUp(() {
testController = GradesTestController();
subRef = testController.service.term(termId).subject(subjectId);
});

SubjectSettingsPageController createPageController() {
return SubjectSettingsPageController(
subRef: subRef,
gradesService: testController.service,
);
}

test(
'regression test: https://github.com/SharezoneApp/sharezone-app/issues/1803\n'
'Correctly adds weights / overrides weights from term', () async {
testController.createTerm(termWith(
id: termId,
finalGradeType: GradeType.schoolReportGrade.id,
gradeTypeWeights: {
GradeType.writtenExam.id: const Weight.factor(1.5),
},
subjects: [
subjectWith(
id: subjectId,
weightType: WeightType.inheritFromTerm,
// Subjects need a grade to be really created/assigned to the term.
grades: [gradeWith()],
),
],
));

var pageController = createPageController();
await pageController.setGradeWeight(
gradeTypeId: GradeType.presentation.id,
weight: const Weight.factor(0.5));

// This controller needs to be recreated, otherwise the bug won't show:
pageController = createPageController();
// This also can't be used instead of recreating the controller to show \
// the bug:
// await pumpEventQueue();

expect(pageController.view.weights.unlockView, {
GradeType.presentation.id: const Weight.factor(0.5),
GradeType.writtenExam.id: const Weight.factor(1.5),
});
expect(pageController.view.finalGradeTypeDisplayName,
GradeType.schoolReportGrade.predefinedType!.toUiString());
});

test('Correctly adds weights', () async {
testController.createTerm(termWith(
id: termId,
finalGradeType: GradeType.schoolReportGrade.id,
gradeTypeWeights: {
GradeType.writtenExam.id: const Weight.factor(1.5),
},
subjects: [
subjectWith(
id: subjectId,
weightType: WeightType.inheritFromTerm,
// Subjects need a grade to be really created/assigned to the term.
grades: [gradeWith()],
),
],
));

var pageController = createPageController();
await pageController.setGradeWeight(
gradeTypeId: GradeType.presentation.id,
weight: const Weight.factor(0.5));

// This works:
expect(pageController.view.weights.unlockView, {
GradeType.presentation.id: const Weight.factor(0.5),
GradeType.writtenExam.id: const Weight.factor(1.5),
});
expect(pageController.view.finalGradeTypeDisplayName,
GradeType.schoolReportGrade.predefinedType!.toUiString());
});

test(
'returns final grade type from term if it is not overridden by subject',
() {
testController.createTerm(
termWith(
id: termId,
finalGradeType: GradeType.presentation.id,
subjects: [
subjectWith(
id: subjectId,
finalGradeType: null,
// Subjects need a grade to be really created/assigned to the term.
grades: [gradeWith()],
),
],
),
);

final pageController = createPageController();

expect(pageController.view.finalGradeTypeDisplayName,
GradeType.presentation.predefinedType!.toUiString());
});

test(
'returns final grade type from subject if it overrides terms final grade type',
() {
testController.createTerm(
termWith(
id: termId,
finalGradeType: GradeType.presentation.id,
subjects: [
subjectWith(
id: subjectId,
finalGradeType: GradeType.writtenExam.id,
// Subjects need a grade to be really created/assigned to the term.
grades: [gradeWith()],
),
],
),
);

final pageController = createPageController();

expect(pageController.view.finalGradeTypeDisplayName,
GradeType.writtenExam.predefinedType!.toUiString());
});

test(
'returns no weights if term has no default weights and subject has no weights',
() async {
testController
.createTerm(termWith(id: termId, gradeTypeWeights: {}, subjects: [
subjectWith(
id: subjectId,
gradeTypeWeights: {},
// Subjects need a grade to be really created/assigned to the term.
grades: [gradeWith()],
),
]));

final pageController = createPageController();

expect(pageController.view.weights, isEmpty);
});
test(
'returns term weights if subjects $WeightType is ${WeightType.inheritFromTerm}',
() async {
testController.createTerm(
termWith(
id: termId,
gradeTypeWeights: {
GradeType.presentation.id: const Weight.factor(0.5),
GradeType.oralParticipation.id: const Weight.percent(200),
},
subjects: [
subjectWith(
id: subjectId,
weightType: WeightType.inheritFromTerm,
// Should be ignored because weights are inherited from term.
gradeTypeWeights: {
GradeType.presentation.id: const Weight.factor(1.5),
},
// Ignore developer warning related to weight settings.
ignoreWeightTypeAssertion: true,
// Subjects need a grade to be really created/assigned to the term.
grades: [gradeWith()],
),
],
),
);

final pageController = createPageController();

expect(pageController.view.weights.unlockView, {
GradeType.presentation.id: const Weight.factor(0.5),
GradeType.oralParticipation.id: const Weight.percent(200),
});
});
test(
'returns subject weights if subjects $WeightType is ${WeightType.perGradeType}',
() async {
testController.createTerm(
termWith(
id: termId,
gradeTypeWeights: {
GradeType.presentation.id: const Weight.factor(0.5),
GradeType.oralParticipation.id: const Weight.percent(200),
},
subjects: [
subjectWith(
id: subjectId,
weightType: WeightType.perGradeType,
// Should be ignored because weights are inherited from term.
gradeTypeWeights: {
GradeType.presentation.id: const Weight.factor(1.5),
},
// Subjects need a grade to be really created/assigned to the term.
grades: [gradeWith()],
),
],
),
);

final pageController = createPageController();

expect(pageController.view.weights.unlockView, {
GradeType.presentation.id: const Weight.factor(1.5),
});
});
test(
'throws if subjects $WeightType is ${WeightType.perGrade} as we dont support it yet',
() async {
testController.createTerm(
termWith(
id: termId,
gradeTypeWeights: {
GradeType.presentation.id: const Weight.factor(0.5),
GradeType.oralParticipation.id: const Weight.percent(200),
},
subjects: [
subjectWith(
id: subjectId,
weightType: WeightType.perGrade,
// Subjects need a grade to be really created/assigned to the term.
grades: [
gradeWith(
value: 10,
gradingSystem: GradingSystem.zeroToFifteenPoints,
),
gradeWith(
value: 12,
gradingSystem: GradingSystem.zeroToFifteenPoints,
weight: const Weight.factor(1.5),
),
],
),
],
),
);

final pageController = createPageController();

expect(pageController.state, isA<SubjectSettingsError>());
});
});
}
Loading