Andrei mirica19/replace year week number #297
Conversation
# Conflicts: # lib/pages/settings/view/settings_page.dart # pubspec.yaml
| }, | ||
| defaultVal: false, | ||
| desc: | ||
| "show number of week in the semester instead of the calendar year", |
There was a problem hiding this comment.
| Only use double quotes for strings containing single quotes |
| visible: authProvider.isAuthenticated && | ||
| !authProvider.isAnonymous && | ||
| authProvider.currentUserFromCache.isAdmin == true, | ||
| visible: authProvider.currentUserFromCache.isAdmin == true, |
There was a problem hiding this comment.
This looks like a broken merge, please revert the condition.
| Column( | ||
| children: [ | ||
| SwitchPreference( | ||
| 'Academic week number ', |
There was a problem hiding this comment.
All user-facing strings should be localized:
https://github.com/student-hub/acs-upb-mobile/blob/master/CONTRIBUTING.md#internationalization
| children: [ | ||
| SwitchPreference( | ||
| 'Academic week number ', | ||
| 'Academic week number', |
There was a problem hiding this comment.
Use a nicer key, like we did above (e.g. academic_week_number.
| }, | ||
| defaultVal: false, | ||
| desc: | ||
| 'show number of week in the semester instead of the calendar year', |
There was a problem hiding this comment.
This should start with uppercase to match the other strings on the page
| // ignore: implementation_imports | ||
| import 'package:timetable/src/theme.dart'; | ||
|
|
||
| // ignore: implementation_imports |
There was a problem hiding this comment.
This comment isn't necessary, this isn't an implementation import.
There was a problem hiding this comment.
You removed the comment above on line 12, which was actually necessary instead of removing this comment. Please pay attention.
| List<ClassHeader> classHeaders = []; | ||
| List<Person> classTeachers = []; | ||
| User user; | ||
| AcademicCalendar calendar; | ||
| Map<String, AcademicCalendar> calendars = {}; | ||
| int academicWeek; | ||
| Set<int> holidayWeeks = {}; |
There was a problem hiding this comment.
Except for maybe calendars, all these class attributes seem unnecessary.
| color: defaultBackgroundColor, | ||
| border: Border.all(color: defaultBackgroundColor, width: 1)), | ||
| child: Text( | ||
| getWeekNumber(), |
There was a problem hiding this comment.
Move the setting check here (see my comment below, I'd like to move the logic of fetching the week to the calendar itself, and the calendar shouldn't know about settings).
| ); | ||
| } | ||
|
|
||
| String getWeekNumber() { |
There was a problem hiding this comment.
Move this to the calendar class directly, maybe make it a getter? Like String get academicWeek (it't not really just a number).
| .then((calendars) { | ||
| setState(() { | ||
| this.calendars = calendars; | ||
| calendar = calendars.values.last; |
There was a problem hiding this comment.
This doesn't work, what if we scroll to the previous year/semester?
We have some existing logic for this in the calendar selection dropdown when adding an event:
https://github.com/student-hub/acs-upb-mobile/blob/master/lib/pages/timetable/view/events/add_event_view.dart#L112-L131
Let's do the following:
- add a
int semesterForDate(DateTime date)method toAcademicCalendarthat does something like this:
for (final semester in semesters) {
if (date.isBeforeOrDuring(semester)) {
// semester.id is represented as "semesterN", where "semester0" is the first semester
return 1 + int.tryParse(semester.id[semester.id.length - 1]);
}
}
return -1;- the code in add_event_view.dart can be simplified to:
final LocalDate date = widget.initialEvent.start.calendarDate ?? LocalDate.today();
calendars.forEach((c) {
final semester = c.semesterForDate(date);
if (semester != -1) {
selectedCalendar = c;
selectedSemester = semester;
return;
}
});- in this file, to fetch the right semester we can just do:
calendars.where((c) => c.semesterForDate(date) != -1).
(Keep in mind I didn't actually compile this code, so try to understand the logic and make changes where necessary)
| @@ -31,6 +32,7 @@ class _SettingsPageState extends State<SettingsPage> { | |||
|
|
|||
| // String describing the level of editing permissions that the user has. | |||
| String userPermissionString = ''; | |||
| bool isSwitched = false; | |||
| import 'package:acs_upb_mobile/pages/people/model/person.dart'; | ||
| import 'package:acs_upb_mobile/pages/people/service/person_provider.dart'; | ||
| import 'package:acs_upb_mobile/pages/timetable/model/academic_calendar.dart'; | ||
| import 'package:acs_upb_mobile/pages/timetable/model/events/all_day_event.dart'; |
There was a problem hiding this comment.
| Unused import: 'package:acs_upb_mobile/pages/timetable/model/events/all_day_event.dart' |
…_number' into AndreiMirica19/Replace_year_week_number
…_number' into AndreiMirica19/Replace_year_week_number
| if (!nonHolidayWeeks.contains(i)) holidayWeeks.add(i); | ||
| } | ||
|
|
||
| if (!nonHolidayWeeks.contains(week)) { |
For more info on versioning, check out our PR guidelines. Generated by 🚫 Danger |
If this is a trivial change that doesn't warrant a test or changelog entry, you can mark it as #trivial in the PR title. Generated by 🚫 Danger |
…_number' into AndreiMirica19/Replace_year_week_number
| import 'package:flutter/material.dart'; | ||
| import 'package:time_machine/time_machine.dart'; | ||
|
|
||
| import 'package:timetable/src/theme.dart'; |
There was a problem hiding this comment.
| Don't import implementation files from another package |
|
|
||
| int semesterForDate(LocalDate date) { | ||
| for (final semester in semesters) { | ||
| if (date._isBeforeOrDuring(semester)) { |
There was a problem hiding this comment.
Please add the comment back, it was relevant.
// semester.id is represented as "semesterN", where "semester0" is the first semester
| final week = ((date.dayOfYear - date.dayOfWeek.value + 10) / 7).floor(); | ||
| if (LeadHeader.academicWeekNumber == false) return week.toString(); |
There was a problem hiding this comment.
AcademicCalendar shouldn't have to know anything about LeadHeader, plus static variables should generally be avoided. This check should be done inside LeadHeader.
You can have two separate methods in this class, e.g. getSemesterWeekNumber and getYearWeekNumber. But honestly, the second method has absolutely nothing to do with the academic calendar and could just be an extension method on LocalDate defined somewhere in LeadHeader or wherever it's needed.
https://en.wikipedia.org/wiki/Single-responsibility_principle
| } | ||
|
|
||
| String getWeekNumber(LocalDate date) { | ||
| final week = ((date.dayOfYear - date.dayOfWeek.value + 10) / 7).floor(); |
There was a problem hiding this comment.
Why this formula? It's not obvious at all what this does, add a comment to explain what's happening here (and maybe below too).
| if (semesterForDate(date) == 1) { | ||
| return (nonHolidayWeeks.toList().indexOf(week) + 1).toString(); | ||
| } else { | ||
| return ((nonHolidayWeeks.toList().indexOf(week) + 1) - |
There was a problem hiding this comment.
I don't like this logic, you're kind of assuming that the holiday between the semesters is exactly one week. This is very likely true for ACS, but if we ever want to extend this, it will lead to a bug that will be tricky to figure out. Similarly, you're also assuming that we have only two semesters :) This is meant to be a generic API.
Instead of calculating the last week of the first semester, why not just get the first week of the each semester, find the index of that in nonHolidayWeeks and simply subtract that from .indexOf(week)? This would be a generic solution that works for any N semesters with arbitrary holidays.
| // ignore: implementation_imports | ||
| import 'package:timetable/src/theme.dart'; | ||
|
|
||
| // ignore: implementation_imports |
There was a problem hiding this comment.
You removed the comment above on line 12, which was actually necessary instead of removing this comment. Please pay attention.
| "settingsTitleLocalization": "Localizare", | ||
| "settingsTitleDataControl": "Control date", | ||
| "settingsItemEditingPermissions": "Permisiunile tale de editare", | ||
| "settingsAcademicWeekNumberSubtitle":"Afișează săptămână din semestru în loc de cea din an" |
There was a problem hiding this comment.
Cred că "săptămâna" sună mai bine decât "săptămână"
| Added: | ||
| -Users can now choose from setting if they want to see in the week indicator,from the timetable,the academic week number. |
There was a problem hiding this comment.
Keep in mind that this is addressed to users. Use the other changelogs as examples. This is as if I was talking to you and saying "Andrei can now pass the maths class". It's usually also good to keep it short, and be consistent with the previous formatting. Furthermore, with every release we need to include all the changes that happened since the last release (this you can usually see from the commit history).
| Added: | |
| -Users can now choose from setting if they want to see in the week indicator,from the timetable,the academic week number. | |
| Added | |
| - The option to show the number of the week in the semester instead of the week in the year. | |
| - A feature to allow admins to send a confirmation email from the app when approving a permission request. | |
| Fixed | |
| - Some minor bugs and text issues. |
Try to do the same thing in the other changelogs as well.
(and also, you'll need to rename them since the version was bumped since the last time you created these files)
| @@ -548,7 +548,7 @@ Future<void> main() async { | |||
| ), | |||
| ]; | |||
| final calendar = AcademicCalendar( | |||
| id: '2020', | |||
| id: '2021', | |||
There was a problem hiding this comment.
Why the change here? The id should be irrelevant, no?
| widget is WeekIndicator && | ||
| widget.week.toString() == previousWeek.toString()), | ||
|
|
||
| await tester.pumpAndSettle(); |
There was a problem hiding this comment.
Why was this line moved?
|
|
||
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(find.byWidgetPredicate((widget) => widget is LeadHeader), |
There was a problem hiding this comment.
This is a very bad change (same for the ones below). It essentially deletes a test that was checking that the weeks are displayed correctly, and instead tests... nothing. This expect call literally does nothing.
Remember: fixing tests that fail does NOT mean deleting the tests that fail :)
Instead, this should specifically test that the LeadHeader works correctly - first, set the setting to show the year week number like it did before (and keep the same logic, with checking the week number using getWeekOfWeekYear). Then copy the same test, change the setting to show academic week and test that it correctly shows the weeks and the holidays.
Now, to actually get the week number from the new widget it's a bit more tricky. I'm not sure what the best approach would be here (maybe the docs have some info about finding text in a specific widget?). I do have one idea, if we want to do something similar to what we had previously (where we're essentially finding the WeekIndicator widget and directly checking its week attribute; this can be done because WeekIndicator is stateless, but in our case LeadHeader is not and we can't directly access this attribute). We can create our own stateless widget (we can even call it WeekIndicator as well) from the Text widget in LeadHeader - basically in LeadHeader have something like:
child:
WeekIndicator(
week: calendar == null ? '' : calendar.getWeekNumber(widget.date),
),
And then WeekIndicator would be a simple StatelessWidget, for which we can directly check the .week attribute. This way, when we use find to get the corresponding widget, we can test that it has the right value.
Again this is just an idea, there may be a better way to do this.
User can choose if they want to see instead of the year week number indicator, the academic week indicator.