-
Notifications
You must be signed in to change notification settings - Fork 108
[MBL-19665][All] - Fix help menu E2E tests (strings and some urls has changed on backend). #3481
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
base: master
Are you sure you want to change the base?
Conversation
refs: MBL-19665 affects: Student, Teacher, Parent release note: -
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
This PR updates help menu URLs and string constants across the Parent, Student, and Teacher apps. The changes migrate from old Canvas community URLs to new Instructure community URLs and update Play Store links for the "Share Love" feature.
Positive Aspects
- Good consistency in updating URLs across all three apps (Parent, Student, Teacher)
- Appropriate use of
market://scheme for Play Store links - String improvements add proper punctuation to subtitles for better consistency
Issues to Address
- Inconsistent capitalization in
StringConstants.kt:54- "conferences in Canvas" uses lowercase "canvas" while it should be capitalized as a proper noun - Test reliability concern in
HelpPage.kt:143-147- Changing fromallOftoanyOfmakes intent matching less strict, which could lead to false positives. This needs clarification or documentation about why the change is necessary.
Additional Observations
The test changes appear related to supporting the new market:// URL scheme, which may behave differently than https:// URLs in intent matching. If this is the reason for the anyOf change, it would be helpful to document this with a code comment for future maintainers.
Overall, this is a straightforward URL update, but the test assertion change needs careful consideration to ensure we're not reducing test effectiveness.
| object Teacher { | ||
| const val CONFERENCE_GUIDES_TITLE = "Conference Guides for Remote Classrooms" | ||
| const val CONFERENCE_GUIDES_SUBTITLE = "Get help on how to use and configure conferences in canvas." | ||
| const val CONFERENCE_GUIDES_SUBTITLE = "Get help on how to use and configure conferences in Canvas." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent capitalization: The subtitle says "conferences in Canvas" but uses lowercase "canvas". Consider using "Canvas" with a capital C to match the proper noun usage throughout the codebase and the title above it.
| val expectedIntent = CoreMatchers.anyOf( | ||
| CoreMatchers.allOf( | ||
| IntentMatchers.hasAction(android.content.Intent.ACTION_VIEW), | ||
| IntentMatchers.hasData(expectedURL) | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test reliability concern: Changing from allOf to anyOf makes the test less strict. This means the test will pass if either the ACTION_VIEW intent OR the bundled extras match, rather than requiring all conditions to be true.
This could lead to false positives where the wrong URL is opened but the test still passes. Consider:
- Keeping
allOfif you need to verify both conditions - Adding a comment explaining why
anyOfis necessary (e.g., if different browsers/Android versions handle intents differently) - Ensuring this change is intentional for the new Play Store URLs that use
market://scheme
🧪 Unit Test Results✅ 📱 Parent App
✅ 📱 Student App
✅ 📱 Teacher App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Fri, 16 Jan 2026 19:00:05 GMT |
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
Fix help menu E2E tests (strings and some urls has changed on backend).
refs: MBL-19665
affects: Student, Teacher, Parent
release note: -