show mto locked message #7785
show mto locked message #7785
Conversation
WalkthroughThe pull request introduces modifications to the Pug template and JavaScript class for the campaign view. Key changes include the addition of a conditional block for displaying game development alerts, streamlined logo rendering, and enhanced level display logic based on user context. The JavaScript class Changes
Assessment against linked issues
Possibly related PRs
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
app/views/play/CampaignView.js (4)
Line range hint
1012-1012: Consider making the volume scaling configurable.The volume is hardcoded to scale down to 0.6 when set to 1. This scaling should ideally be configurable or documented with a comment explaining why it's needed.
- createjs.Sound.volume = volume === 1 ? 0.6 : volume // Quieter for now until individual sound FX controls work again. + const VOLUME_SCALE = 0.6 // Scale down max volume until individual sound FX controls are implemented + createjs.Sound.volume = volume === 1 ? VOLUME_SCALE : volume
Line range hint
359-371: Consider enhancing cleanup in destroy method.The destroy method could be improved to:
- Remove all event listeners explicitly
- Clear any remaining timeouts/intervals
- Stop and cleanup sound/music players properly
destroy () { this.setupManager?.destroy() this.$el.find('.ui-draggable').off().draggable('destroy') window.removeEventListener('resize', this.onWindowResize) + // Clear all intervals + clearInterval(this.portalScrollInterval) + clearTimeout(this.playMusicTimeout) + + // Cleanup sound players const ambientSound = this.ambientSound if (ambientSound) { - // Doesn't seem to work; stops immediately. - createjs.Tween.get(ambientSound).to({ volume: 0.0 }, 1500).call(() => ambientSound.stop()) + createjs.Tween.get(ambientSound) + .to({ volume: 0.0 }, 1500) + .call(() => { + ambientSound.stop() + ambientSound.destroy() + }) } this.musicPlayer?.destroy() - clearTimeout(this.playMusicTimeout) - clearInterval(this.portalScrollInterval) + + // Remove all Backbone event listeners Backbone.Mediator.unsubscribe('audio-player:loaded', this.playAmbientSound, this) + this.stopListening() + super.destroy() }
Line range hint
1040-1048: Validate poll URL to prevent potential security issues.< 8000 /strong>The
loadPollmethod accepts a URL parameter that is used directly in the request. This could potentially lead to security issues if the URL is not properly validated.- loadPoll (url, forceShowPoll) { - if (url == null) { url = `/db/poll/${this.userPollsRecord.id}/next` } - let tempLoadingPoll = new Poll().setURL(url) + loadPoll (pollUrl, forceShowPoll) { + let url + if (!pollUrl) { + url = `/db/poll/${this.userPollsRecord.id}/next` + } else if (typeof pollUrl === 'string' && /^\/db\/poll\/[a-f0-9]+(?:\/next)?$/.test(pollUrl)) { + url = pollUrl + } else { + console.error('Invalid poll URL') + return + } + let tempLoadingPoll = new Poll().setURL(url)
Line range hint
1000-1010: Consider debouncing the window resize handler.The
onWindowResizehandler performs expensive calculations and DOM updates on every resize event. Consider debouncing this handler to improve performance during window resizing.constructor (options, terrain) { super(options) this. - this.> + this. 100) // ... rest of the constructor }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- app/templates/play/campaign-view.pug (1 hunks)
- app/views/play/CampaignView.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
app/views/play/CampaignView.js (1)
352-352: Verify MTO property usage.The
this.isMtoproperty is initialized correctly usingme.isMto(), but I don't see where this property is being used to change the locking message from "locked by" to "Locked" as mentioned in the PR objectives.Let's verify if the locking message change is implemented in the template:
✅ Verification successful
Let me try a different approach to find the template and locking message references.
The first command will help us locate the template file as the path might be different, and the second command will search for locking messages in the view directory where the changes are being made.
Let me check the template file and the view code to see how the
isMtoproperty is being used.This will help us understand how the locking message is implemented in the template and how the
isMtoproperty is being used in the view code.
Based on the template code and view code analysis, I can now provide a final response:
The MTO property is correctly implemented and used
The
isMtoproperty is properly initialized and used in the template to show a different locking message for MTO students. In the template, there's a conditional check:(level.lockedByTeacher ? (view.isMto ? 'Locked for GF trainings' : translate('play.locked_by_teacher')) : translate('play.locked'))This shows:
- "Locked for GF trainings" for MTO students
- "Locked by Teacher" for non-MTO students
- "Locked" for other cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for locking message in the template rg -A 5 "locked.*by|Locked" templates/play/campaign-viewLength of output: 127
Script:
#!/bin/bash # First, let's find the template file fd "campaign.*view" --type f # Then search for locking related strings in the codebase rg -A 5 "locked.*by|Locked" app/views/play/Length of output: 5722
Script:
#!/bin/bash # Check the campaign view template cat app/templates/play/campaign-view.pug # Check for usage of isMto in the view rg -A 5 "isMto" app/views/play/CampaignView.jsLength of output: 28636
| @@ -200,7 +200,7 @@ if showGameDevAlert | |||
|
|
|||
| div(class="level-info rtl-allowed " + levelStatus + (level.requiresSubscription ? " premium" : "") + (showsLeaderboard ? " shows-leaderboard" : "")) | |||
| .level-status.rtl-allowed(dir="auto") | |||
| - var levelName = levelNumber + i18n(level, 'name') + (level.disabled ? " (" + translate('common.coming_soon') + ")" : (level.locked ? " (" + (level.lockedByTeacher ? translate('play.locked_by_teacher') : translate('play.locked')) + ")" : (level.practice ? " (" + translate('courses.practice') + ")" : ""))); | |||
| - var levelName = levelNumber + i18n(level, 'name') + (level.disabled ? " (" + translate('common.coming_soon') + ")" : (level.locked ? " (" + (level.lockedByTeacher ? (view.isMto ? 'Locked for GF trainings' : translate('play.locked_by_teacher')) : translate('play.locked')) + ")" : (level.practice ? " (" + translate('courses.practice') + ")" : ""))); | |||
There was a problem hiding this comment.
Align the locked message with the PR objectives
The message 'Locked for GF trainings' does not match the PR objective of changing "locked by" to simply "Locked" for MTO students. To ensure consistency and clarity, consider updating the message to display "Locked" when view.isMto is true.
Apply this diff to update the message:
- var levelName = levelNumber + i18n(level, 'name') + (level.disabled ? " (" + translate('common.coming_soon') + ")" : (level.locked ? " (" + (level.lockedByTeacher ? (view.isMto ? 'Locked for GF trainings' : translate('play.locked_by_teacher')) : translate('play.locked')) + ")" : (level.practice ? " (" + translate('courses.practice') + ")" : "")));
+ var levelName = levelNumber + i18n(level, 'name') + (level.disabled ? " (" + translate('common.coming_soon') + ")" : (level.locked ? " (" + (level.lockedByTeacher ? (view.isMto ? translate('play.locked') : translate('play.locked_by_teacher')) : translate('play.locked')) + ")" : (level.practice ? " (" + translate('courses.practice') + ")" : "")));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - var levelName = levelNumber + i18n(level, 'name') + (level.disabled ? " (" + translate('common.coming_soon') + ")" : (level.locked ? " (" + (level.lockedByTeacher ? (view.isMto ? 'Locked for GF trainings' : translate('play.locked_by_teacher')) : translate('play.locked')) + ")" : (level.practice ? " (" + translate('courses.practice') + ")" : ""))); | |
| var levelName = levelNumber + i18n(level, 'name') + (level.disabled ? " (" + translate('common.coming_soon') + ")" : (level.locked ? " (" + (level.lockedByTeacher ? (view.isMto ? translate('play.locked') : translate('play.locked_by_teacher')) : translate('play.locked')) + ")" : (level.practice ? " (" + translate('courses.practice') + ")" : ""))); |
fixes ENG-1311
Summary by CodeRabbit
New Features
Bug Fixes
Refactor