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 calendar not loading when calendar context limit is increased #2991

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vargaat
Copy link
Contributor

@vargaat vargaat commented Nov 29, 2024

What changed?

  • There was a breaking API change in /api/v1/settings/environment.json: the calendar_contexts_limit field no longer returns a bool whether it's enabled or not but returns the actual limit as an integer in case the Increase calendar context limit toggle is enabled. If this setting is not enabled the API won't return this field at all.
  • I changed how we store this setting and from now on we use the limit returned by the API instead of the hardcoded 20 value.

refs: MBL-18102
affects: Student, Teacher, Parent
release note: Fixed calendar not displaying the filter list and events properly.

test plan:

  • Enable "Increase calendar context limit" in course settings on web.
  • Go to all apps, check if calendar loads.
  • Check if calendar filter loads, pull to refresh doesn't show an error page.
  • Test if teacher app still has a functioning calendar filter (max 10/20 events).

Checklist

  • Tested on phone
  • Tested on tablet

refs: MBL-18102
affects: Student, Teacher, Parent
release note: Fixed calendar not displaying the filter list and events properly.

test plan:
- Enable "Increase calendar context limit" in course settings on web.
- Go to all apps, check if calendar loads.
- Check if calendar filter loads, pull to refresh doesn't show an error page.
- Test if teacher app still has a functioning calendar filter (max 10/20 events).
@vargaat vargaat changed the title Update environment settings API call according to the new format Fix calendar not loading when calendar context limit is increased Nov 29, 2024
@inst-danger
Copy link
Contributor

Parent Build QR Code:

@inst-danger
Copy link
Contributor

Teacher Build QR Code:

@inst-danger
Copy link
Contributor

Student Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Nov 29, 2024

Warnings
⚠️ One or more files are below the minimum test coverage 50%

Release Note:

Fixed calendar not displaying the filter list and events properly.

Affected Apps: Student, Teacher, Parent

MBL-18102

Coverage New % Master % Delta
Canvas iOS 90.11% 90.11% -0%
Core/Core/Planner/CalendarMain/ViewModel/PlannerViewModel.swift 0% 0% 0%
Core/Core/Planner/CalendarMain/Model/CalendarWeek.swift 0% 0% 0%
Core/Core/Planner/CalendarMain/View/Calendar/CalendarCardInteractionState.swift 0% 0% 0%
Core/Core/Planner/CalendarMain/Model/PlannablesInteractor.swift 0% 0% 0%
Core/Core/Planner/CalendarMain/View/Calendar/ViewPreferences.swift 0% 0% 0%
Core/Core/Planner/CalendarMain/Model/CalendarDay.swift 0% 0% 0%
Core/Core/Planner/CalendarMain/Model/CalendarMonth.swift 0% 0% 0%
Core/Core/Planner/CalendarMain/View/Calendar/CalendarUtils.swift 0% 0% 0%

Generated by 🚫 dangerJS against a2b9647

Copy link
Contributor

@balintbartok balintbartok left a comment

Choose a reason for hiding this comment

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

QA + 1

@vargaat vargaat self-assigned this Nov 29, 2024
case .extended(let value): return value
case .unlimited: return 9999
}
}
}

public extension Optional where Wrapped == AppEnvironment.App {
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that on this extension, if we haveisCalendarFilterLimitEnabled always evaluated to false for Student app. I am wondering whether we need for Student or Parent apps to still worry about parsing this property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, I also thought about this but left it as-is to save some time but it would be a nice future improvement.

Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst left a comment

Choose a reason for hiding this comment

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

QA + 1
Code + 1

Thanks @vargaat !
But I have few concerns for you to consider.

@@ -17,7 +17,9 @@
//

public struct GetEnvironmentSettingsRequest: APIRequestable {
public typealias Response = [String: Bool]
public struct Response: Codable {
public let calendar_contexts_limit: Int?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we say the it is confirmed the BE team will continue using this for future?
Or, perhaps, would be better to keep a logic to check for the Bool value as well? (The old way).

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.

4 participants