changes to codeninja club/camp creation - show option of esports #7911
Conversation
WalkthroughThis pull request introduces enhancements to the classroom management system, focusing on Code Ninja classroom types. The changes span multiple files, adding new methods to the Changes
Sequence DiagramsequenceDiagram
participant User
participant ModalEditClass
participant Classroom
participant ClubCampMixin
User->>ModalEditClass: Select Classroom Type
ModalEditClass->>Classroom: Get CodeNinja Classroom Types
Classroom-->>ModalEditClass: Return Classroom Types
ModalEditClass->>ClubCampMixin: Validate Classroom Type
ClubCampMixin->>ModalEditClass: Validate Duration
ModalEditClass->>User: Display Validation Results
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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: 0
🧹 Nitpick comments (6)
ozaria/site/components/teacher-dashboard/mixins/clubCampMixin.js (1)
5-7: Avoid duplicated logic withClassroommodel.
Currently, this method duplicates the logic found inClassroom.isCodeNinjaClubCamp(). Since the model already offers the same check, consider callingclassroom.isCodeNinjaClubCamp()directly or referencing the model’s method in one place to reduce maintenance overhead.- isCodeNinjaClubCamp (classroom) { - return Classroom.codeNinjaClassroomTypes().map(type => type.id).includes(classroom?.type) - }, + isCodeNinjaClubCamp (classroom) { + return classroom?.isCodeNinjaClubCamp?.() ?? false + },ozaria/site/components/teacher-dashboard/BaseMyClasses/index.vue (1)
248-248: Prop naming alignment.
:as-club="isCodeNinjaClubCamp(editClassroomObject)"clearly communicates intent. Ensure future references consistently useas-clubto avoid confusion with “camp” scenarios.ozaria/site/components/teacher-dashboard/modals/modal-edit-class-components/CourseCodeLanguageFormatComponent.vue (1)
258-258: Possible duplication inhideCodeLanguageAndFormat.
You’re filtering club/camp types in multiple places (model, mixin, and now here). Consider a single, reusable approach (e.g., callingClassroom.codeNinjaClassroomTypes()or a helper) to keep these lists synced.app/models/Classroom.js (1)
495-504: Export these new classroom types from a central config.
Defining them inline is fine for now, but if you foresee expansions or shared usage across modules, consider extracting them into a central constants file for easier maintenance.ozaria/site/components/teacher-dashboard/modals/ModalEditClass.vue (2)
127-127: Consider localizing the label.
Appending" (As Club / Camp)"works but may bypass your i18n/l10n best practices. Consider defining or reusing an i18n token for better international support.
297-300: Date range validation improvements.
The new restrictions for camp (5 days) and club (14 weeks) are a clear, logical approach. However, consider:
• Timezone edge cases (e.g., partial boundary day).
• Aligning error messages with your i18n framework.
• Whether you want inclusive vs. exclusive bounds (14 weeks plus 1 day?).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/models/Classroom.js(1 hunks)app/templates/courses/classroom-settings-modal.pug(0 hunks)ozaria/site/components/teacher-dashboard/BaseMyClasses/index.vue(3 hunks)ozaria/site/components/teacher-dashboard/BaseTeacherDashboard/index.vue(3 hunks)ozaria/site/components/teacher-dashboard/mixins/clubCampMixin.js(1 hunks)ozaria/site/components/teacher-dashboard/modals/ModalEditClass.vue(3 hunks)ozaria/site/components/teacher-dashboard/modals/modal-edit-class-components/CourseCodeLanguageFormatComponent.vue(2 hunks)
💤 Files with no reviewable changes (1)
- app/templates/courses/classroom-settings-modal.pug
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node.js CI (18.10.0)
🔇 Additional comments (9)
ozaria/site/components/teacher-dashboard/BaseMyClasses/index.vue (1)
Line range hint
12-28: Great addition of the club/camp mixin.
ImportingclubCampMixinand mixing it in alongsidesortClassroomMixinhelps keep this component lean. The approach is clean and organized.ozaria/site/components/teacher-dashboard/modals/modal-edit-class-components/CourseCodeLanguageFormatComponent.vue (2)
158-164: Good conditional rendering for single code format.
Allowing a disabled<input>when only one code format is available streamlines the UX. No issues found.
166-166: Ensure accessibility for dropdown.
While this conditional fallback is great, ensure the<select>has appropriate labeling or ARIA attributes for assistive technologies (if needed by your product’s accessibility requirements).app/models/Classroom.js (1)
506-508: Method naming synergy.
isCodeNinjaClubCamp()is consistent with the new mixin’s logic, but having them in both places can cause confusion. Make sure to unify usage where possible.ozaria/site/components/teacher-dashboard/BaseTeacherDashboard/index.vue (4)
24-24: Import looks good.
Importing theclubCampMixinhere is well-structured and logically separated.
46-46: Consistent component registration.
RegisteringTopBannerfollows the existing pattern. No issues found.
48-48: Proper use of mixins.
IncludingclubCampMixinin themixinsarray is a clean approach for reusing club/camp-related logic across components.
463-463: VerifyisClubusage.
Ensure thatisClubis defined and properly handles all relevant classroom types. If it references the mixin, confirm it correctly identifiesclubvs.campor other custom types.ozaria/site/components/teacher-dashboard/modals/ModalEditClass.vue (1)
171-171: Good extraction of club types.
ReferencingClassroom.codeNinjaClassroomTypes()centralizes the club/camp type definitions, which aids maintainability and consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/core/api/background-job.js(2 hunks)app/core/store/modules/apiClient.js(2 hunks)app/lib/common-utils.js(1 hunks)ozaria/site/components/teacher-dashboard/BaseMyClasses/index.vue(4 hunks)ozaria/site/components/teacher-dashboard/BaseTeacherDashboard/index.vue(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ozaria/site/components/teacher-dashboard/BaseTeacherDashboard/index.vue
🔇 Additional comments (5)
ozaria/site/components/teacher-dashboard/BaseMyClasses/index.vue (3)
12-12: LGTM! Verify the clubCampMixin implementation.The mixin is properly imported and integrated. The ordering with
sortClassroomMixinfirst is correct.Let's verify the mixin implementation:
Also applies to: 27-28
✅ Verification successful
Verified: clubCampMixin implementation is correct and properly used
The mixin provides a clean implementation of
isCodeNinjaClubCampmethod and is correctly integrated in both BaseMyClasses and BaseTeacherDashboard components. The implementation aligns with the Code Ninja classroom types functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the clubCampMixin implementation and its usage # Check the mixin implementation echo "Checking clubCampMixin implementation..." rg -p "clubCampMixin" --type vue --type js # Check for isCodeNinjaClubCamp method usage echo "Checking isCodeNinjaClubCamp usage..." ast-grep --pattern 'isCodeNinjaClubCamp($_)'Length of output: 293
Script:
#!/bin/bash # Search for clubCampMixin implementation and usage echo "=== Searching for clubCampMixin files ===" fd -e js -e vue clubCampMixin echo "=== Checking content of clubCampMixin ===" fd -e js -e vue clubCampMixin -x cat {} echo "=== Searching for references to clubCampMixin ===" rg "clubCampMixin" -g "*.{js,vue}" echo "=== Checking for isCodeNinjaClubCamp method ===" rg "isCodeNinjaClubCamp" -g "*.{js,vue}"Length of output: 1539
248-248: LGTM! Verify the as-club prop handling.The
:as-clubprop is properly bound to determine if the classroom is a Code Ninja club/camp.Let's verify the prop handling in ModalEditClass:
✅ Verification successful
Prop implementation verified and working as expected
The
as-clubprop is properly defined in ModalEditClass and used consistently for:
- Conditional validation requirements
- Club-specific UI elements
- Proper prop forwarding to child components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the as-club prop implementation in ModalEditClass # Check ModalEditClass component for as-club prop definition echo "Checking as-club prop definition..." rg -p "as-club|asClub" --type vue # Check for conditional rendering based on as-club echo "Checking conditional rendering..." ast-grep --pattern 'v-if="$_club"'Length of output: 258
Script:
#!/bin/bash # Description: Verify the as-club prop implementation with improved patterns # Add .vue as a type and search for prop echo "Checking as-club prop definition..." rg -p --type-add 'vue:*.vue' -t vue '(as-club|asClub)' # Check for prop definition in component options echo "Checking prop in component options..." ast-grep --pattern 'props: { $$$ asClub: $_, $$$ }' # Check template conditions echo "Checking template conditions..." ast-grep --pattern '<template> $$$ <$_ $$$ :as-club="$_" $$$ /> $$$ </template>'Length of output: 1676
133-134: Verify the impact of disabling student creation.The method now always returns
false, which disables student creation functionality entirely. This is a significant change that might affect user workflows.Let's verify the impact:
Consider adding a code comment explaining why student creation is disabled:
showCreateStudents (_classroom) { + // Student creation disabled for Code Ninja club/camp classrooms return false },✅ Verification successful
Verified: Student creation is intentionally disabled for Code Ninja classrooms
The change is part of the Code Ninja club/camp classroom implementation, where automatic student creation is disabled. The component uses
clubCampMixinand the related API endpoint (createStudentsForCodeNinja) confirms this is specific to Code Ninja classrooms.Consider adding a code comment to document this intentional restriction:
showCreateStudents (_classroom) { + // Student creation is disabled for Code Ninja club/camp classrooms return false },🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage and potential impact of showCreateStudents # Check for components using modal-add-students echo "Checking modal-add-students usage..." rg -p "modal-add-students" --type vue # Check for references to showCreateStudents echo "Checking showCreateStudents usage..." rg -p "showCreateStudents|createStudents" --type vueLength of output: 314
Script:
#!/bin/bash # Search for showCreateStudents method usage and related UI elements echo "Checking showCreateStudents usage..." rg -p "showCreateStudents" -A 2 -B 2 echo "\nChecking create students related UI elements..." rg -p "create-students|createStudents|add-students" -A 2 -B 2 echo "\nChecking the original file..." cat "ozaria/site/components/teacher-dashboard/BaseMyClasses/index.vue"Length of output: 16368
app/lib/common-utils.js (1)
6-8: LGTM! Clean Promise-based sleep implementation.The sleep function follows the standard pattern for Promise-based delays and is properly implemented.
app/core/store/modules/apiClient.js (1)
60-62: Verify the background job API contract.Let's verify the structure of the background job response to ensure consistent handling across the codebase.
✅ Verification successful
Background job API contract is consistent ✓
The implementation in apiClient.js correctly follows the established background job contract used across the codebase, with proper job creation, status handling, and result processing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all background job API usages to verify consistent contract handling # Look for job creation patterns echo "Checking job creation patterns:" rg -A 2 "createJob|create as createJob" --type js # Look for job status access patterns echo "\nChecking job status access patterns:" rg -B 2 -A 2 "job\.status|job\.job|job\.output|job\.message" --type jsLength of output: 4526
app/core/api/background-job.js
Outdated
| async function pollTillResult (jobId, other, options = {}) { | ||
| let url = `/db/background-jobs/${jobId}` | ||
| if (other) { | ||
| url = '/' + other + url | ||
| } | ||
| let job = await fetchJson(url, options) | ||
| while (job.status !== 'completed' && job.status !== 'failed') { | ||
| await sleep(5000) | ||
| job = await fetchJson(url, options) | ||
| } | ||
| if (job.status === 'completed') { | ||
| return JSON.parse(job.output) | ||
| } | ||
| if (job.status === 'failed') { | ||
| throw new Error(job.message) | ||
| } | ||
| 8000 | } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and backoff mechanism to prevent excessive polling.
While the implementation is functional, it could be improved to handle edge cases better:
-async function pollTillResult (jobId, other, options = {}) {
+async function pollTillResult (jobId, other, options = {}) {
+ const MAX_ATTEMPTS = 60; // 5 minutes with 5s interval
+ const BASE_DELAY = 5000;
+ let attempts = 0;
let url = `/db/background-jobs/${jobId}`
if (other) {
url = '/' + other + url
}
let job = await fetchJson(url, options)
- while (job.status !== 'completed' && job.status !== 'failed') {
- await sleep(5000)
+ while (job.status !== 'completed' && job.status !== 'failed' && attempts < MAX_ATTEMPTS) {
+ attempts++;
+ // Exponential backoff with max delay of 30 seconds
+ const delay = Math.min(BASE_DELAY * Math.pow(1.1, attempts - 1), 30000);
+ await sleep(delay)
job = await fetchJson(url, options)
}
+ if (attempts >= MAX_ATTEMPTS) {
+ throw new Error('Polling timeout exceeded')
+ }
if (job.status === 'completed') {
- return JSON.parse(job.output)
+ try {
+ return job.output ? JSON.parse(job.output) : null
+ } catch (e) {
+ throw new Error(`Failed to parse job output: ${e.message}`)
+ }
}
if (job.status === 'failed') {
throw new Error(job.message)
}
}📝 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.
| async function pollTillResult (jobId, other, options = {}) { | |
| let url = `/db/background-jobs/${jobId}` | |
| if (other) { | |
| url = '/' + other + url | |
| } | |
| let job = await fetchJson(url, options) | |
| while (job.status !== 'completed' && job.status !== 'failed') { | |
| await sleep(5000) | |
| job = await fetchJson(url, options) | |
| } | |
| if (job.status === 'completed') { | |
| return JSON.parse(job.output) | |
| } | |
| if (job.status === 'failed') { | |
| throw new Error(job.message) | |
| } | |
| } | |
| async function pollTillResult (jobId, other, options = {}) { | |
| const MAX_ATTEMPTS = 60; // 5 minutes with 5s interval | |
| const BASE_DELAY = 5000; | |
| let attempts = 0; | |
| let url = `/db/background-jobs/${jobId}` | |
| if (other) { | |
| url = '/' + other + url | |
| } | |
| let job = await fetchJson(url, options) | |
| while (job.status !== 'completed' && job.status !== 'failed' && attempts < MAX_ATTEMPTS) { | |
| attempts++; | |
| // Exponential backoff with max delay of 30 seconds | |
| const delay = Math.min(BASE_DELAY * Math.pow(1.1, attempts - 1), 30000); | |
| await sleep(delay) | |
| job = await fetchJson(url, options) | |
| } | |
| if (attempts >= MAX_ATTEMPTS) { | |
| throw new Error('Polling timeout exceeded') | |
| } | |
| if (job.status === 'completed') { | |
| try { | |
| return job.output ? JSON.parse(job.output) : null | |
| } catch (e) { | |
| throw new Error(`Failed to parse job output: ${e.message}`) | |
| } | |
| } | |
| if (job.status === 'failed') { | |
| throw new Error(job.message) | |
| } | |
| } |
app/core/store/modules/apiClient.js
Outdated
| fetchLicenseStats: async ({ commit }, { clientId, startDate, endDate }) => { | ||
| commit('toggleLoading', 'byLicense') | ||
| return apiclientsApi | ||
| .getLicenseStats(clientId, { startDate, endDate }) | ||
| .then(res => { | ||
| if (res) { | ||
| commit('addLicenseStats', { | ||
| stats: res | ||
| }) | ||
| } else { | ||
| throw new Error('Unexpected response from license stats by owner API.') | ||
| } | ||
| const job = await createJob('api-client-stats', { clientId, startDate, endDate }) | ||
| try { | ||
| const res = await pollTillResult(job?.job) | ||
| commit('addLicenseStats', { | ||
| stats: res, | ||
| }) | ||
| .catch((e) => noty({ text: 'Fetch license stats failure: ' + e, type: 'error' })) | ||
| .finally(() => commit('toggleLoading', 'byLicense')) | ||
| } catch (err) { | ||
| noty({ text: 'Fetch license stats failure: ' + err, type: 'error' }) | ||
| } finally { | ||
| commit('toggleLoading', 'byLicense') | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and loading state management.
The async/await implementation is good, but there are a few improvements needed:
fetchLicenseStats: async ({ commit }, { clientId, startDate, endDate }) => {
commit('toggleLoading', 'byLicense')
- const job = await createJob('api-client-stats', { clientId, startDate, endDate })
try {
+ const job = await createJob('api-client-stats', { clientId, startDate, endDate })
+ if (!job || !job.job) {
+ throw new Error('Failed to create background job')
+ }
- const res = await pollTillResult(job?.job)
+ const res = await pollTillResult(job.job)
commit('addLicenseStats', {
stats: res,
})
} catch (err) {
- noty({ text: 'Fetch license stats failure: ' + err, type: 'error' })
+ noty({
+ text: `Failed to fetch license stats: ${err.message || 'Unknown error'}`,
+ type: 'error'
+ })
} finally {
commit('toggleLoading', 'byLicense')
}
},📝 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.
| fetchLicenseStats: async ({ commit }, { clientId, startDate, endDate }) => { | |
| commit('toggleLoading', 'byLicense') | |
| return apiclientsApi | |
| .getLicenseStats(clientId, { startDate, endDate }) | |
| .then(res => { | |
| if (res) { | |
| commit('addLicenseStats', { | |
| stats: res | |
| }) | |
| } else { | |
| throw new Error('Unexpected response from license stats by owner API.') | |
| } | |
| const job = await createJob('api-client-stats', { clientId, startDate, endDate }) | |
| try { | |
| const res = await pollTillResult(job?.job) | |
| commit('addLicenseStats', { | |
| stats: res, | |
| }) | |
| .catch((e) => noty({ text: 'Fetch license stats failure: ' + e, type: 'error' })) | |
| .finally(() => commit('toggleLoading', 'byLicense')) | |
| } catch (err) { | |
| noty({ text: 'Fetch license stats failure: ' + err, type: 'error' }) | |
| } finally { | |
| commit('toggleLoading', 'byLicense') | |
| } | |
| fetchLicenseStats: async ({ commit }, { clientId, startDate, endDate }) => { | |
| commit('toggleLoading', 'byLicense') | |
| try { | |
| const job = await createJob('api-client-stats', { clientId, startDate, endDate }) | |
| if (!job || !job.job) { | |
| throw new Error('Failed to create background job') | |
| } | |
| const res = await pollTillResult(job.job) | |
| commit('addLicenseStats', { | |
| stats: res, | |
| }) | |
| } catch (err) { | |
| noty({ | |
| text: `Failed to fetch license stats: ${err.message || 'Unknown error'}`, | |
| type: 'error' | |
| }) | |
| } finally { | |
| commit('toggleLoading', 'byLicense') | |
| } | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/views/api/components/ApiData.vue (2)
6-6: Consider simplifying the condition logic.The negated condition
!hideByStudentTab()makes the code slightly harder to read. Consider renaming the method toshouldShowByStudentTab()to avoid the negation.- .tab(v-if="!hideByStudentTab()" @click="tab = 'byStudent'" :class="{active: tab === 'byStudent'}") By Student + .tab(v-if="shouldShowByStudentTab()" @click="tab = 'byStudent'" :class="{active: tab === 'byStudent'}") By Student
9-10: Enhance link accessibility and security.A few improvements needed:
- The text before the link should use i18n
- The external link needs security attributes
- p(class="link-info") Access the new dashboard with graphs - a(href="/partner-dashboard?fromOld=1" target="_blank") {{ $t('general.here') }} + p(class="link-info") {{ $t('dashboard.access_new_dashboard') }} + a(href="/partner-dashboard?fromOld=1" target="_blank" rel="noopener noreferrer") {{ $t('general.here') }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/components/common/Navigation.vue(1 hunks)app/views/api/components/ApiData.vue(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node.js CI (18.10.0)
🔇 Additional comments (4)
app/components/common/Navigation.vue (2)
111-111: LGTM! The change is consistent with the account dropdown menu.The new navigation path to '/api-dashboard' aligns with the existing API dashboard link in the account dropdown menu, which is shown when
me.isAPIClient()is true.Also applies to: 392-393
111-111: Verify the precedence of navigation paths for Code Ninja users.The new condition for Code Ninja API clients is placed before the condition for Code Ninja teachers. Please verify that this order aligns with the expected user experience, as it means API clients will be directed to the API dashboard even if they are also teachers.
✅ Verification successful
The routing precedence for Code Ninja users is correctly implemented.
The order of conditions in the navigation logic is appropriate as the roles (student/API client/teacher) are mutually exclusive, and each Code Ninja user will be directed to their respective dashboard based on their specific role.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other Code Ninja-related routing logic rg -A 5 "isCodeNinja\(\)" --type vue --type jsLength of output: 76
Script:
#!/bin/bash # Search for Code Ninja and API client related logic echo "=== Searching for isCodeNinja() calls ===" rg "isCodeNinja\(\)" echo -e "\n=== Searching for isAPIClient() calls ===" rg "isAPIClient\(\)" echo -e "\n=== Searching for API dashboard routes ===" rg "/api-dashboard"Length of output: 6609
app/views/api/components/ApiData.vue (2)
202-203: LGTM!The xTicks configuration is properly set to disable tick marks on the x-axis, which is appropriate for categorical data like age ranges.
225-225: LGTM!The initialization of the tab based on client type is correct and consistent with the component's logic.
app/views/api/components/ApiData.vue
Outdated
| isGeccClient () { | ||
| this.myId = me.get('_id') | ||
| const geccId = '61e7e20658f1020024bd8cf7' | ||
| return this.myId === geccId | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor client identification logic.
Several issues with the current implementation:
- The GECC client ID is hardcoded
- The method has a side effect of setting myId
- No type checking or error handling for the ID comparison
+ // Move to constants/configuration
+ const GECC_CLIENT_ID = '61e7e20658f1020024bd8cf7'
+
+ data () {
+ return {
+ tab: 'byMonth',
+ spiedUser: window.serverSession.amActually,
+ myId: me.get('_id') // Move to data
+ }
+ },
isGeccClient () {
- this.myId = me.get('_id')
- const geccId = '61e7e20658f1020024bd8cf7'
- return this.myId === geccId
+ return this.myId === GECC_CLIENT_ID
},Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/core/api/background-job.js (1)
24-47: 🛠️ Refactor suggestionEnhance polling mechanism with better error handling and backoff strategy.
The current implementation could be improved in several areas:
- Add exponential backoff to prevent overwhelming the server
- Add robust error handling for JSON parsing
- Improve timeout error message with more context
Apply these improvements:
async function pollTillResult (jobId, other, options = {}) { let url = `/db/background-jobs/${jobId}` if (other) { url = '/' + other + url } let job = await fetchJson(url, options) - const MAX_ATTEMPTS = 30 - const DELAY_MS = 5000 + const MAX_ATTEMPTS = 60 // 5 minutes with initial 5s delay + const BASE_DELAY = 5000 let attempts = 0 while (job.status !== 'completed' && job.status !== 'failed' && attempts < MAX_ATTEMPTS) { - await sleep(DELAY_MS) + attempts++ + // Exponential backoff with max delay of 30 seconds + const delay = Math.min(BASE_DELAY * Math.pow(1.1, attempts - 1), 30000) + await sleep(delay) job = await fetchJson(url, options) - attempts++ } if (job.status === 'completed') { - return JSON.parse(job.output) + try { + return job.output ? JSON.parse(job.output) : null + } catch (e) { + throw new Error(`Failed to parse job output: ${e.message}`) + } } if (job.status === 'failed') { throw new Error(job.message) } - if (attempts === MAX_ATTEMPTS) { - throw new Error('Failed to load the results') + if (attempts >= MAX_ATTEMPTS) { + throw new Error(`Polling timeout exceeded after ${MAX_ATTEMPTS} attempts for job ${jobId}`) } }
🧹 Nitpick comments (4)
app/views/api/components/ApiData.vue (4)
6-6: Convert method to computed property for better performance.The
hideByStudentTab()method is used in a template conditional and would be better implemented as a computed property since it depends on user state.- .tab(v-if="!hideByStudentTab()" @click="tab = 'byStudent'" :class="{active: tab === 'byStudent'}") By Student + .tab(v-if="showByStudentTab" @click="tab = 'byStudent'" :class="{active: tab === 'byStudent'}") By Student // In computed section: + showByStudentTab () { + return me.isGeccClient() + }
9-10: Enhance accessibility of the dashboard link.The link to the new dashboard lacks accessibility attributes and could be more descriptive.
- p(class="link-info") Access the new dashboard with graphs - a(href="/partner-dashboard?fromOld=1" target="_blank") {{ $t('general.here') }} + p(class="link-info") + a(href="/partner-dashboard?fromOld=1" + target="_blank" + rel="noopener noreferrer" + aria-label="Access the new dashboard with detailed graphs in a new tab") + | Access the new dashboard with graphs {{ $t('general.here') }}
186-193: Simplify watch handler using object shorthand.The watch block can be simplified using object shorthand syntax and early return.
watch: { - clientId: function (id) { - if (id !== '') { - this.fetchTeachers(id) - this.fetchPrepaids({ teacherId: this.myId, clientId: id }) - this.fetchLicenseStats({ clientId: id }) - } - }, + clientId (id) { + if (!id) return + this.fetchTeachers(id) + this.fetchPrepaids({ teacherId: this.myId, clientId: id }) + this.fetchLicenseStats({ clientId: id }) + }, },
200-203: Remove commented code.The commented code about play time stats should be removed if it's no longer needed. If it might be needed in the future, add a TODO comment explaining why it's commented out.
this.fetchClientId() - // current play time for apiclient is the total time of all students so i think - // we doesn't need it now - /* this.fetchPlayTimeStats() */ + // TODO: Implement individual student play time tracking instead of total time
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/core/api/background-job.js(2 hunks)app/core/store/modules/apiClient.js(2 hunks)app/models/User.js(1 hunks)app/views/api/components/ApiData.vue(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/core/store/modules/apiClient.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node.js CI (18.10.0)
🔇 Additional comments (4)
app/views/api/components/ApiData.vue (1)
227-229: Convert method to computed property.This method should be a computed property as previously suggested in the template section.
app/core/api/background-job.js (3)
49-53: LGTM!The module exports are clean and follow modern JavaScript conventions.
24-47: Verify integration with consuming code.Let's ensure the new
pollTillResultfunction is properly integrated with its consumers.✅ Verification successful
Integration verification completed successfully
The
pollTillResultfunction is properly integrated and centrally used for background job polling, with no duplicate implementations found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of pollTillResult rg "pollTillResult" --type js # Check for similar polling patterns that might need to be cons A93C olidated rg -A 5 "while.*status.*!==.*completed" --type jsLength of output: 823
Line range hint
16-19: Consider addressing the TODO comment.The comment suggests moving the URL construction logic into
fetchJson. This refactoring would improve code maintainability by centralizing URL handling logic.Let's check if this URL construction pattern is used elsewhere:
✅ Verification successful
The TODO comment highlights a valid opportunity for improvement
The URL construction pattern
url = '/' + other + urlappears in multiple API files. Moving this logic tofetchJsonwould:
- Centralize URL manipulation alongside existing URL processing logic
- Reduce code duplication across API modules
- Make URL construction more maintainable
Consider adding an option similar to
callOzinfetchJsonto handle this common URL prefix pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar URL construction patterns rg -g '*.js' "url = '/' \+ .* \+ url"Length of output: 276
Script:
#!/bin/bash # Check fetchJson implementation and its imports ast-grep --pattern 'function fetchJson' rg -g '*.js' "import.*fetchJson"Length of output: 1533
Script:
#!/bin/bash # Find fetch-json implementation fd fetch-json.js cat app/core/api/fetch-json.jsLength of output: 2453
| isGeccClient () { | ||
| const GECC_ID = '61e7e20658f1020024bd8cf7' | ||
| return this.get('_id') === GECC_ID | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the GECC client identification implementation.
The current implementation has several issues that should be addressed:
+ // Constants
+ static GECC_CLIENT_ID = '61e7e20658f1020024bd8cf7'
+
+ /**
+ * Checks if the current user is a GECC client
+ * @returns {boolean} True if user is GECC client, false otherwise
+ */
isGeccClient () {
- const GECC_ID = '61e7e20658f1020024bd8cf7'
- return this.get('_id') === GECC_ID
+ const userId = this.get('_id')
+ if (!userId) return false
+ return userId === User.GECC_CLIENT_ID
}The refactoring:
- Moves the client ID to a static class constant
- Adds JSDoc documentation
- Adds null check for user ID
📝 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.
| isGeccClient () { | |
| const GECC_ID = '61e7e20658f1020024bd8cf7' | |
| return this.get('_id') === GECC_ID | |
| } | |
| // Constants | |
| static GECC_CLIENT_ID = '61e7e20658f1020024bd8cf7' | |
| /** | |
| * Checks if the current user is a GECC client | |
| * @returns {boolean} True if user is GECC client, false otherwise | |
| */ | |
| isGeccClient () { | |
| const userId = this.get('_id') | |
| if (!userId) return false | |
| return userId === User.GECC_CLIENT_ID | |
| } |
Summary by CodeRabbit
Release Notes
New Features
Improvements
User Interface