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: different schedules in consecutive weeks #1170

Merged
merged 26 commits into from
Mar 1, 2024

Conversation

limwa
Copy link
Member

@limwa limwa commented Feb 28, 2024

This PR fixes an issue where schedules of consecutive weeks were merged and displayed all on the same weekday.

This happens because when we fetch the lectures from the API, we send (in today's case) pv_semana_ini=20240228 and pv_semana_fim=20240305. This is the equivalent of a week starting today. The problem arises because, for those parameters, the API returns the lectures between 25-02-2024 and 09-03-2024, since those are the 2 blocks that intercept the range of dates we asked for.

To fix this, we now issue 1 request per week (the smallest block possible) to be able to determine the full date of each class (the API only returns the weekday).

This also required changes to the widgets to support these new functionalities.

PR-specific Tasks

  • Achieve desired functionality in API fetcher
  • Achieve desired functionality in HTML fetcher
  • Refactor code to make it more reusable and understandable

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

@bdmendes bdmendes added this to the March 2023 Release milestone Feb 28, 2024
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Merging #1170 (5bd8dd0) into develop (a5ad009) will increase coverage by 1%.
Report is 2 commits behind head on develop.
The diff coverage is 74%.

Additional details and impacted files
@@           Coverage Diff           @@
##           develop   #1170   +/-   ##
=======================================
+ Coverage       15%     16%   +1%     
=======================================
  Files          228     230    +2     
  Lines         6907    7003   +96     
=======================================
+ Hits          1027    1086   +59     
- Misses        5880    5917   +37     

@limwa limwa requested a review from a team February 29, 2024 15:42
@limwa limwa enabled auto-merge February 29, 2024 17:28
Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work overall, but it was a difficult bug fix overall (dates are really difficult to work with properly) 🚀 🚀 just have some concerns below.

uni/lib/view/schedule/schedule.dart Outdated Show resolved Hide resolved
uni/lib/view/schedule/schedule.dart Outdated Show resolved Hide resolved
uni/lib/view/schedule/schedule.dart Outdated Show resolved Hide resolved
uni/lib/model/utils/week_response.dart Outdated Show resolved Hide resolved
@limwa limwa disabled auto-merge March 1, 2024 11:48
@limwa
Copy link
Member Author

limwa commented Mar 1, 2024

Disabled auto-merge because I still need to test these latest changes

@limwa limwa force-pushed the limwa/fix/different-schedules-consecutive-weeks branch from 1ab0694 to c8fa63c Compare March 1, 2024 14:37
Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 🚀 🚀

@limwa limwa enabled auto-merge March 1, 2024 14:49
Copy link
Member

@bdmendes bdmendes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. Below you can find some nitpicky comments.

uni/lib/model/utils/time/week.dart Outdated Show resolved Hide resolved
uni/lib/model/utils/time/weekday_mapper.dart Show resolved Hide resolved
uni/lib/view/schedule/schedule.dart Outdated Show resolved Hide resolved
@limwa limwa requested a review from bdmendes March 1, 2024 20:04
@limwa limwa merged commit 39a66e5 into develop Mar 1, 2024
6 checks passed
@limwa limwa deleted the limwa/fix/different-schedules-consecutive-weeks branch March 1, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants