Conversation
…and functionality - Moved score-related utility functions and constants to score_view_helpers.dart. - Created score_screen_actions.dart for handling score refresh and last updated caching. - Updated ScoreScreen to utilize new helper functions and actions for better readability and maintainability. - Enhanced academic performance provider to streamline data loading and caching logic.
Dokploy Preview Deployment
|
PR Preview BuildsBuild Number: 322 Android (Firebase App Distribution) |
- add a TODO for the empty onTap of _tableOwnerIndicator - change _tableOwnerIndicator private [skip ci]
…and functionality - Moved score-related utility functions and constants to score_view_helpers.dart. - Created score_screen_actions.dart for handling score refresh and last updated caching. - Updated ScoreScreen to utilize new helper functions and actions for better readability and maintainability. - Enhanced academic performance provider to streamline data loading and caching logic.
…udent-service' into score-screen-and-student-service
|
@kevinlee-06 What will happen if the average and GPA data are not available yet? |
There was a problem hiding this comment.
Pull request overview
Adds a functional “Scores” screen backed by an offline-first data pipeline that fetches/normalizes academic performance + GPA from NTUT services and persists them locally for fast rendering.
Changes:
- Extend
StudentQueryServiceto (a) more robustly parse academic performance tables and (b) fetch/parse per-semester cumulative GPA. - Introduce score screen UI + Riverpod provider/actions for cache-first loading, pull-to-refresh, and “last updated” timestamp handling.
- Add Drift schema support for persisting historical cumulative GPA per semester and migrate DB to schema v2.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/services/student_query_service.dart | Improves score parsing strategy; adds GPA fetch + HTML parsing; adds session-expiry detection. |
| lib/screens/main/score/score_view_helpers.dart | Adds formatting + display helpers for the score UI. |
| lib/screens/main/score/score_screen_actions.dart | Adds refresh + last-updated timestamp persistence helpers. |
| lib/screens/main/score/score_screen.dart | Implements the score UI with semester tabs, refresh, summary card, and score list. |
| lib/screens/main/score/score_providers.dart | Implements cache-first StreamProvider that reads from Drift, refreshes via SSO, enriches course names, and persists results. |
| lib/database/schema.dart | Adds grandTotalGpa to UserSemesterSummaries. |
| lib/database/database.g.dart | Regenerated Drift code for schema change. |
| lib/database/database.dart | Bumps schema version and adds migration to create the new GPA column. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int findPreferredSemesterIndex( | ||
| List<SemesterScoreDto> semesters, | ||
| String? selectedSemesterKey, | ||
| ) { | ||
| if (selectedSemesterKey == null) return -1; | ||
| return semesters.indexWhere((semester) { | ||
| return semesterKey(semester) == selectedSemesterKey; | ||
| }); | ||
| } | ||
|
|
||
| int findDefaultSemesterIndex(List<SemesterScoreDto> semesters) { | ||
| final index = semesters.indexWhere((semester) => semester.scores.isNotEmpty); | ||
| return index >= 0 ? index : 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
findPreferredSemesterIndex / findDefaultSemesterIndex are not used anywhere in the codebase (ScoreScreen has its own private implementations). Consider removing these helpers or updating ScoreScreen to reuse them so we don’t keep dead/duplicated logic around.
| int findPreferredSemesterIndex( | |
| List<SemesterScoreDto> semesters, | |
| String? selectedSemesterKey, | |
| ) { | |
| if (selectedSemesterKey == null) return -1; | |
| return semesters.indexWhere((semester) { | |
| return semesterKey(semester) == selectedSemesterKey; | |
| }); | |
| } | |
| int findDefaultSemesterIndex(List<SemesterScoreDto> semesters) { | |
| final index = semesters.indexWhere((semester) => semester.scores.isNotEmpty); | |
| return index >= 0 ? index : 0; | |
| } |
| import 'package:tattoo/models/score.dart'; | ||
| import 'package:tattoo/models/user.dart'; | ||
| import 'package:tattoo/utils/http.dart'; | ||
| import 'package:tattoo/services/portal_service.dart'; |
There was a problem hiding this comment.
portal_service.dart is imported but only referenced in doc comments, which will still trigger an unused import lint. Consider removing this import, or adjusting the doc comment to avoid requiring the symbol link/import (or add an explicit usage if you really need the dartdoc link).
| import 'package:tattoo/services/portal_service.dart'; |
| /// Returns a list of [GradeRankingDto] ordered from most recent to oldest, | ||
| /// each containing ranking positions at class, group, and department levels. | ||
| /// Returns ranking entries including class, group, and department levels. | ||
| /// Semester information is spans multiple ranking type rows using `rowspan`. |
There was a problem hiding this comment.
Docstring grammar issue: "Semester information is spans ..." is ungrammatical and should be reworded (e.g., "Semester information spans ..." or "Semester information is spanned across ...").
| /// Semester information is spans multiple ranking type rows using `rowspan`. | |
| /// Semester information spans multiple ranking type rows using `rowspan`. |
| /// Fetches grand total GPA records by semester. | ||
| Future<List<GpaDto>> getGPA() async { | ||
| final response = await _studentQueryDio.get('QryGPA.jsp'); | ||
|
|
||
| if (response.data.toString().contains('應用系統已中斷連線')) { | ||
| throw Exception('SessionExpired'); | ||
| } | ||
|
|
||
| return _parseGpaFromDocument(parse(response.data)); | ||
| } |
There was a problem hiding this comment.
getGPA and _parseGpaFromDocument introduce new parsing behavior (including dedupe + sorting) but there are no corresponding tests in test/services/student_query_service_test.dart. Since this repo already has integration tests for StudentQueryService, please add coverage for at least: (1) returns non-empty GPA rows, (2) semesters are ordered most-recent-first, and (3) values parse as doubles.
| _selectedIndex = activeIndex >= semesters.length ? 0 : activeIndex; | ||
| _selectedSemesterKey = semesterKey(semesters[_selectedIndex]); |
There was a problem hiding this comment.
State is being mutated during build (_selectedIndex / _selectedSemesterKey). Mutating State fields in build without setState is error-prone and can lead to hard-to-debug UI inconsistencies. Prefer deriving local variables from semesters + controller, and update persistent selection state only from the tab controller listener (or via setState outside of the build phase).
| _selectedIndex = activeIndex >= semesters.length ? 0 : activeIndex; | |
| _selectedSemesterKey = semesterKey(semesters[_selectedIndex]); | |
| final newIndex = activeIndex >= semesters.length ? 0 : activeIndex; | |
| final newSelectedSemesterKey = semesterKey(semesters[newIndex]); | |
| if (newIndex != _selectedIndex || | |
| newSelectedSemesterKey != _selectedSemesterKey) { | |
| WidgetsBinding.instance.addPostFrameCallback((_) { | |
| if (!mounted) return; | |
| setState(() { | |
| _selectedIndex = newIndex; | |
| _selectedSemesterKey = newSelectedSemesterKey; | |
| }); | |
| }); | |
| } |
Re: @Dao-you In cases where the GPA data is unavailable, the |
fa0966c to
96e5de1
Compare
342fd3e to
4276526
Compare
65b9f61 to
6892297
Compare

No description provided.