CC-1539: Fix start-track button #2511
Conversation
Updated the start-track-button component to include a 'repo' query param when transitioning to the course route if the user is not anonymous. This change allows for better handling of repository information in the transition.
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/components/track-page/start-track-button.ts (1)
33-33: Consider adding a fallback ifthis.args.coursesis empty.
While the non-null assertion!indicates that you expectthis.args.coursesto contain at least one course, it can throw errors if that ever changes. For robustness, consider adding a check for an empty array to prevent errors in edge cases.You could handle this more defensively, for example:
@action handleClicked() { if (this.currentUserIsAnonymous) { this.authenticator.initiateLogin(null); } else { + if (!this.args.courses?.length) { + // Fallback: either return early or handle the missing course scenario + return; + } this.router.transitionTo('course', this.args.courses[0]!.slug, { queryParams: { repo: null, track: this.args.language.slug }, }); } }
❌ 3 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/acceptance/track-page/start-track-test.js (4)
11-16: Grammar Improvement in Test TitleThe test name "it display the start-track-button" can be phrased in a more grammatically correct way, e.g. "it displays the start-track-button…”.
- test('it display the start-track-button for anonymous user', async function (assert) { + test('it displays the start-track-button for anonymous user', async function (assert) {
18-24: Custom AssertionsConsider adding an additional assertion after a button click to ensure it behaves correctly for a logged-in user who has not started any course in the track, confirming that it transitions correctly to the next page or step.
26-41: Informative Happy Path TestIt might be valuable to also verify that subsequent visits or actions (like re-clicking the button) do not incorrectly re-initiate the track or re-insert the
repoquery param. This helps guard against regressions.
43-62: Ensuring Clarity on Query ParamsThe line:
assert.notOk(currentURL().includes('repo='));correctly confirms that the
repoquery param is not present. Consider an explicit assertion or comment describing the significance of removingrepo, since it’s closely tied to the root cause of the original bug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/track-page/start-track-button.hbs(1 hunks)tests/acceptance/track-page/start-track-test.js(1 hunks)tests/pages/track-page.js(1 hunks)
🔇 Additional comments (3)
app/components/track-page/start-track-button.hbs (1)
1-1: Consistent Test Identifier Update
Changing the attribute to data-test-primary-start-track-button looks consistent with the naming used in the tests. Ensure that any references to the older identifier across the codebase and tests are fully updated.
tests/pages/track-page.js (2)
16-16: Good Addition
Using a dedicated clickable property for the "start track" button helps keep the test code expressive and maintainable. This approach aligns well with page object best practices.
18-18: Visibility Check is Great
Introducing hasStartTrackButton provides a clear way to assert the button's presence. This is a straightforward solution that keeps the test code concise.
Clear
repoinqueryParams, so that previously created repos won't interfere with the result of clicking Start Track button again.Checklist:
[percy]in the message to trigger)Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes