Feat | Score rules#35
Conversation
… amount was already fulfilled
ryukzak
left a comment
There was a problem hiding this comment.
PR Review: Score Rules
Critical Issues
1. Duplicate logic: CalculateScoreEffects and CalculateTotalEffect are identical
config.go has two methods that do the exact same thing. Only CalculateTotalEffect is used. CalculateScoreEffects should be removed.
2. Duplicate rule evaluation logic between config.go and scorerule.go
Config.RuleApplies() in config.go duplicates the logic in Evaluator.EvaluateForStudent() in scorerule.go. The config method is only called via CalculateTotalEffect (used in users.go for the user list/CSV). This means the same rule can produce different results from two different code paths — a divergence bug waiting to happen.
Recommendation: Remove RuleApplies and CalculateScoreEffects/CalculateTotalEffect from config.go. Use the Evaluator everywhere.
3. N+1 query problem in user list
In UserListHandler, for each student, getCheckedTime is called per rule per task, and each call does DB.ListTaskRecords. For a class of 30 students with 6 rules × 4 tasks, that's potentially 720 DB reads on every page load. Consider batching — load all records for a student once, then filter in memory.
Same pattern repeats in UserInfoHandler and UserListCSVHandler.
4. time.Now() called in business logic makes testing impossible
RuleApplies and EvaluateForStudent both call time.Now() internally. This makes the rule evaluation non-deterministic and untestable. Pass now as a parameter instead.
Medium Issues
5. Error silently swallowed in evaluator
In scorerule.go:39-41, when getCheckedTime returns an error, the task is silently skipped. In config.go, same pattern but with a comment "Error equals task was not checked" — which is misleading since actual DB errors are also swallowed.
6. Unused fields added in the diff
TaskViewModel.CompletedCountis added totask.gobutcompletedCountis always 0 — never assigned.TaskViewModel.CurrentTimeis added but never used intask.html.User.RuleAppliesis set but only accessed in the template via$.RuleApplies— yet the template already has.StatusonScoreRuleWithStatus, makingRuleAppliesredundant.
7. hasTask check is redundant
In user.go, the loop checks if a score rule's task IDs exist in AppConfig.Tasks. But LoadConfig already validates this — it returns an error if a rule references a non-existent task. This check is dead code.
8. GetCompletedTasksCountInGroup in db.go is unused
The new method added to storage/db.go is never called anywhere.
Minor / Style
9. Template formatting changes are noisy — The PR mixes feature work with whitespace reformatting ({{ define vs {{define, etc.) across multiple templates. These should be in a separate commit.
10. Template indentation is broken in user.html — The score rules table progressively loses indentation towards the bottom. Closing </table>, </div>, </section> are not at the correct indent level.
11. docker-compose.yaml hardcodes JWT secret — SLAP_JWT_SECRET=test in the main compose file; consider docker-compose.override.yaml or .env instead.
12. Double --- separator in GUIDE.md — produces a redundant horizontal rule.
No Tests
The PR adds significant business logic (rule evaluation with multiple condition branches) but includes zero tests — no unit tests for RuleApplies/EvaluateForStudent, no hurl integration tests.
Summary
| Category | Count |
|---|---|
| Critical | 4 |
| Medium | 4 |
| Minor | 4 |
Key asks before merge:
- Remove duplicate logic — single code path for rule evaluation
- Add unit tests for rule evaluation (especially edge cases around
time.Now()) - Add hurl tests for the score rules UI
- Fix the N+1 query pattern or add a TODO acknowledging it
- Remove dead code (
CalculateScoreEffects,CompletedCount,GetCompletedTasksCountInGroup)


By this pull request new extra points system is introduced - Score Rules. There is new mechanics, score rules, which allows admin to assign bonuses & penalties for specific tasks on specific conditions with specific amount of points.
Resolves #34 , where detailed info might be found