wip edlink #8013
wip edlink #8013
Conversation
WalkthroughA new OAuth2 login handler for ClassLink was introduced, mirroring the refactored Schoology login flow to use a shared Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthModal
participant SchoologyHandler
participant ClassLinkHandler
participant EdlinkBaseHandler
participant OAuthProvider
User->>AuthModal: Clicks Schoology or ClassLink login button
AuthModal->>SchoologyHandler: onClickSchoologyLoginButton()
AuthModal->>ClassLinkHandler: onClickClasslinkLoginButton()
SchoologyHandler->>EdlinkBaseHandler: logInWithEdlink()
ClassLinkHandler->>EdlinkBaseHandler: logInWithEdlink()
EdlinkBaseHandler->>OAuthProvider: Open OAuth popup with state
OAuthProvider-->>EdlinkBaseHandler: Write result to localStorage
EdlinkBaseHandler->>AuthModal: Resolve login result
AuthModal->>User: Reload page or show login failure
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.1)app/locale/en.js🔧 ESLint
npm warn EBADENGINE Unsupported engine { ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 9
🧹 Nitpick comments (4)
app/styles/modal/auth-modal.sass (2)
250-259: Hard-coded width risks truncation in i18n – prefermin-widthChanging
gapto5pxis fine, but the button still uses a strictwidth: 130px. Longer translations (e.g. German, Finnish) routinely overflow. Consider switching tomin-widthso the button can grow when necessary..signup-with-button @@ - width: 130px + min-width: 130px
170-175: Duplicate declarations for#schoology-login-btn– consolidate to one block
#schoology-login-btnis styled twice (lines 170-175 and 260-264). The second block overrides the first border colour, increasing maintenance cost and cognitive load.Merge the common rules into a single selector (now that
#classlink-login-btnshares them) to follow DRY:-#schoology-login-btn - z-index: 100 - border: 1px solid black - background-color: white - color: black +// shared styling +#schoology-login-btn, +#classlink-login-btn + z-index: 100 + border: 1px solid #117bbc + background-color: white + color: blackThen delete the later duplicate block.
Also applies to: 260-264
app/templates/core/create-account-modal/basic-info-view.pug (1)
31-36: Consider localization consistency for social login buttons.The Schoology button changed from using
data-i18nto hardcoded text "Schoology", and ClassLink also uses hardcoded "ClassLink". While this might be intentional for brand consistency, consider whether these provider names should be localized or kept as-is across all locales.app/views/core/AuthModal.js (1)
328-328: Improve error handling specificity.Consider providing more specific error messages for different failure scenarios rather than a generic "login failed" message. This would improve user experience by helping users understand what went wrong.
- noty({ text: $.i18n.t('login.login_failed'), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true }) + noty({ text: $.i18n.t('login.edlink_login_failed'), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true })Alternatively, pass the provider name to create more specific error messages:
+ const providerName = handler.constructor.name.replace('Handler', '') + noty({ text: $.i18n.t('login.provider_login_failed', { provider: providerName }), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true })Also applies to: 338-338
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/assets/images/pages/modal/auth/classlink-logo-small.pngis 8000 excluded by!**/*.pngapp/assets/images/pages/modal/auth/classlink-logo-text.pngis excluded by!**/*.pngapp/assets/images/pages/modal/auth/clever-logo-blue.pngis excluded by!**/*.png
📒 Files selected for processing (9)
app/core/social-handlers/ClassLinkHandler.js(1 hunks)app/core/social-handlers/EdlinkBaseHandler.js(1 hunks)app/core/social-handlers/SchoologyHandler.js(1 hunks)app/locale/en.js(1 hunks)app/styles/modal/auth-modal.sass(2 hunks)app/styles/modal/create-account-modal/basic-info-view.sass(2 hunks)app/templates/core/auth-modal.pug(2 hunks)app/templates/core/create-account-modal/basic-info-view.pug(1 hunks)app/views/core/AuthModal.js(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/core/social-handlers/ClassLinkHandler.js (2)
app/core/social-handlers/SchoologyHandler.js (1)
EdlinkBaseHandler(1-1)app/views/core/AuthModal.js (2)
require(21-21)ClassLinkHandler(23-23)
app/core/social-handlers/EdlinkBaseHandler.js (3)
app/core/social-handlers/SchoologyHandler.js (1)
EdlinkBaseHandler(1-1)app/core/social-handlers/ClassLinkHandler.js (1)
EdlinkBaseHandler(1-1)app/lib/user-utils.js (1)
localStorage(2-2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ESLint CI
- GitHub Check: Node.js CI (20.19.2)
🔇 Additional comments (8)
app/core/social-handlers/ClassLinkHandler.js (1)
1-7: Clean implementation following the established pattern.The ClassLinkHandler correctly extends EdlinkBaseHandler with the appropriate provider name. The implementation is consistent with the refactored approach used for other Edlink providers.
app/styles/modal/create-account-modal/basic-info-view.sass (2)
37-55: Good styling consistency for Edlink-based providers.The grouped styling for
.schoology-login,.classlink-loginand their respective buttons is appropriate since both providers now share the same OAuth2 base handler implementation. The consistent styling reinforces their similar functionality.
61-65: Visual improvements for Clever logo styling.The updated Clever logo styling with background, padding, and border creates better visual consistency with other social login buttons.
app/templates/core/create-account-modal/basic-info-view.pug (1)
27-27: Updated Clever logo image source.The image source change from
clever_sso_button@2x.pngtoclever-logo-blue.pngaligns with the styling updates in the SASS file.app/templates/core/auth-modal.pug (3)
81-81: Updated Clever logo image source for consistency.The image source change aligns with the updates in the create-account modal and styling files.
87-89: ClassLink integration follows established pattern.The new ClassLink login button implementation is consistent with the existing social login button pattern and integrates well with the new handler architecture.
122-122: Clever SSO button image asset exists and is correctly referencedVerified that the
clever_sso_button@2x.pngfile lives at
app/assets/images/pages/modal/auth/clever_sso_button@2x.png
which aligns with thesrc="/images/pages/modal/auth/clever_sso_button@2x.png"reference inapp/templates/core/auth-modal.pug. No further action required.app/views/core/AuthModal.js (1)
43-43: LGTM! Event handler properly added for ClassLink button.The event handler follows the existing pattern and correctly maps to the new login method.
| .clever-login-img | ||
| height: 43px | ||
| background: white | ||
| padding: 5px | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Padding increases rendered height – add box-sizing to avoid visual mis-alignment
The element has a fixed height: 43px, yet the new padding: 5px will make the rendered box 53 px tall unless box-sizing: border-box is applied. On small screens this can break vertical centring inside the .network-login button.
.clever-login-img
height: 43px
background: white
padding: 5px
+ box-sizing: border-box // keep total height at 43 px🤖 Prompt for AI Agents
In app/styles/modal/auth-modal.sass around lines 232 to 236, the
.clever-login-img class has a fixed height of 43px and padding of 5px, which
increases the rendered height and can break vertical alignment. To fix this, add
box-sizing: border-box to the .clever-login-img style so padding is included
within the fixed height, preserving the intended layout.
| gplus_linked: 'Google Account linked successfully', | ||
| plus_link_error: 'Google Account linked failed', | ||
| email_or_username_or_library: 'Email or Username or Library Card number', | ||
| schoology_login_failed: 'Login failed. You may need to sign up first.', | ||
| schoology_signup: 'Sign Up with Schoology', | ||
| login_failed: 'Login failed. You may need to sign up first.', | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify duplicate-key collision and cross-locale consistency
login_failed is a good generic replacement, but please confirm:
- The same key does not already exist elsewhere in this file (duplicate object keys are silently overwritten in JS).
- The now-unused keys (
schoology_login_failed,schoology_signup) have been removed from all locale files and the codebase, otherwise dead code and fallback warnings may surface. - Other locale files define
login_failed; otherwise they will fall back to English.
Quick check script:
🏁 Script executed:
#!/bin/bash
# 1. look for duplicate keys in en.js
rg --line-number $'login_failed:' app/locale/en.js
# 2. ensure old keys are gone project-wide
rg --line-number -e $'schoology_login_failed' -e $'schoology_signup' app/locale
# 3. list locales missing the new key
fd -e js app/locale | while read f; do
if ! rg -q $'login_failed:' "$f"; then echo "⚠️ missing in $f"; fi
doneLength of output: 12108
To verify that every locale file defines the new login_failed key, please run:
🏁 Script executed:
#!/bin/bash
# 3. list any locale files missing the new key
rg -L "login_failed:" -g '*.js' app/localeLength of output: 6524
Cleanup obsolete schoology_* keys and propagate login_failed to all locales
Our checks revealed:
- en.js has a single
login_failedentry (no duplicates). schoology_login_failed/schoology_signupare still present (commented) in nearly every locale and actively defined in rot13.js.- Many locale files lack the new
login_failedkey and will silently fall back to English.
Please:
- Remove every
schoology_login_failedandschoology_signupline (including the active entries in rot13.js and all commented blocks). - Add
login_failed: 'Login failed. You may need to sign up first.',
to each locale file missing it. - Verify with:
to confirm no stale keys remain and every locale defines
rg -N "schoology_login_failed|schoology_signup" app/locale rg -L "login_failed:" -g '*.js' app/locale
login_failed.
🤖 Prompt for AI Agents
In app/locale/en.js around lines 704 to 708, remove all occurrences of the
obsolete keys `schoology_login_failed` and `schoology_signup` from all locale
files including rot13.js and any commented blocks. Then, ensure that the
`login_failed` key with the value 'Login failed. You may need to sign up first.'
is added to every locale file that currently lacks it. Finally, run the provided
ripgrep commands to verify no stale keys remain and all locales define
`login_failed`.
| const state = Math.random().toString(36).substring(2) | ||
| const url = `/auth/oauth2/${this.providerName}?state=${state}` | ||
|
|
||
| window.open(url, '_blank', 'width=800,height=600', false) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for popup window opening.
The popup window might be blocked by browsers or fail to open, which should be handled gracefully.
- window.open(url, '_blank', 'width=800,height=600', false)
+ const popup = window.open(url, '_blank', 'width=800,height=600', false)
+ if (!popup) {
+ throw new Error('Popup blocked - please allow popups for this site')
+ }📝 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.
| window.open(url, '_blank', 'width=800,height=600', false) | |
| const popup = window.open(url, '_blank', 'width=800,height=600', false) | |
| if (!popup) { | |
| throw new Error('Popup blocked - please allow popups for this site') | |
| } |
🤖 Prompt for AI Agents
In app/core/social-handlers/EdlinkBaseHandler.js at line 13, the call to
window.open does not handle the case where the popup might be blocked or fail to
open. Modify the code to check the return value of window.open, and if it is
null or undefined, handle the error gracefully by logging a warning or notifying
the user that the popup could not be opened.
| } | ||
|
|
||
| async logInWithEdlink () { | ||
| const state = Math.random().toString(36).substring(2) |
There was a problem hiding this comment.
Use cryptographically secure random generation for OAuth state.
The current implementation uses Math.random() which is not cryptographically secure and could lead to state prediction attacks in OAuth flows.
- const state = Math.random().toString(36).substring(2)
+ const state = crypto.getRandomValues(new Uint32Array(1))[0].toString(36)📝 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.
| const state = Math.random().toString(36).substring(2) | |
| - const state = Math.random().toString(36).substring(2) | |
| + const state = crypto.getRandomValues(new Uint32Array(1))[0].toString(36) |
🤖 Prompt for AI Agents
In app/core/social-handlers/EdlinkBaseHandler.js at line 10, replace the use of
Math.random() for generating the OAuth state with a cryptographically secure
method. Use Node.js's crypto module, such as crypto.randomBytes, to generate a
secure random string for the state parameter to prevent prediction attacks.
| const result = localStorage.getItem(connectionTrackingKey) | ||
| const parsedResult = JSON.parse(result) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JSON parsing.
The JSON parsing could fail if the localStorage data is corrupted or malformed.
const result = localStorage.getItem(connectionTrackingKey)
- const parsedResult = JSON.parse(result)
+ let parsedResult
+ try {
+ parsedResult = JSON.parse(result)
+ } catch (parseError) {
+ window.removeEventListener('storage', handleStorageEvent)
+ reject(new Error('Invalid OAuth response format'))
+ return
+ }📝 Committable suggestion
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.
| const result = localStorage.getItem(connectionTrackingKey) | |
| const parsedResult = JSON.parse(result) | |
| const result = localStorage.getItem(connectionTrackingKey) | |
| let parsedResult | |
| try { | |
| parsedResult = JSON.parse(result) | |
| } catch (parseError) { | |
| window.removeEventListener('storage', handleStorageEvent) | |
| reject(new Error('Invalid OAuth response format')) | |
| return | |
| } |
🤖 Prompt for AI Agents
In app/core/social-handlers/EdlinkBaseHandler.js around lines 20 to 21, the
JSON.parse call on localStorage data can throw an error if the data is corrupted
or malformed. Wrap the JSON.parse call in a try-catch block to handle any
parsing errors gracefully. In the catch block, handle the error appropriately,
such as logging the error and setting parsedResult to a safe default value.
| return new Promise((resolve, reject) => { | ||
| const handleStorageEvent = (event) => { | ||
| if (event.key === connectionTrackingKey) { | ||
| const result = localStorage.getItem(connectionTrackingKey) | ||
| const parsedResult = JSON.parse(result) | ||
| if (parsedResult.state === state) { | ||
| localStorage.removeItem(connectionTrackingKey) | ||
| window.removeEventListener('storage', handleStorageEvent) | ||
| const { state, ...rest } = parsedResult | ||
| resolve(rest) | ||
| } else { | ||
| reject(new Error(`State mismatch: ${parsedResult.state} !== ${state}`)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| window.addEventListener('storage', handleStorageEvent) | ||
| } catch (error) { | ||
| reject(error) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve event listener cleanup and error handling.
The current implementation has potential memory leaks if the promise is rejected before the storage event fires, and the event listener isn't cleaned up in all error scenarios.
return new Promise((resolve, reject) => {
+ let timeoutId
const handleStorageEvent = (event) => {
if (event.key === connectionTrackingKey) {
const result = localStorage.getItem(connectionTrackingKey)
const parsedResult = JSON.parse(result)
if (parsedResult.state === state) {
localStorage.removeItem(connectionTrackingKey)
window.removeEventListener('storage', handleStorageEvent)
+ if (timeoutId) clearTimeout(timeoutId)
const { state, ...rest } = parsedResult
resolve(rest)
} else {
+ window.removeEventListener('storage', handleStorageEvent)
+ if (timeoutId) clearTimeout(timeoutId)
reject(new Error(`State mismatch: ${parsedResult.state} !== ${state}`))
}
}
}
try {
window.addEventListener('storage', handleStorageEvent)
+ // Add timeout to prevent hanging promises
+ timeoutId = setTimeout(() => {
+ window.removeEventListener('storage', handleStorageEvent)
+ reject(new Error('OAuth login timeout'))
+ }, 300000) // 5 minutes
} catch (error) {
reject(error)
}
})📝 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.
| return new Promise((resolve, reject) => { | |
| const handleStorageEvent = (event) => { | |
| if (event.key === connectionTrackingKey) { | |
| const result = localStorage.getItem(connectionTrackingKey) | |
| const parsedResult = JSON.parse(result) | |
| if (parsedResult.state === state) { | |
| localStorage.removeItem(connectionTrackingKey) | |
| window.removeEventListener('storage', handleStorageEvent) | |
| const { state, ...rest } = parsedResult | |
| resolve(rest) | |
| } else { | |
| reject(new Error(`State mismatch: ${parsedResult.state} !== ${state}`)) | |
| } | |
| } | |
| } | |
| try { | |
| window.addEventListener('storage', handleStorageEvent) | |
| } catch (error) { | |
| reject(error) | |
| } | |
| }) | |
| return new Promise((resolve, reject) => { | |
| let timeoutId | |
| const handleStorageEvent = (event) => { | |
| if (event.key === connectionTrackingKey) { | |
| const result = localStorage.getItem(connectionTrackingKey) | |
| const parsedResult = JSON.parse(result) | |
| if (parsedResult.state === state) { | |
| localStorage.removeItem(connectionTrackingKey) | |
| window.removeEventListener('storage', handleStorageEvent) | |
| if (timeoutId) clearTimeout(timeoutId) | |
| const { state, ...rest } = parsedResult | |
| resolve(rest) | |
| } else { | |
| window.removeEventListener('storage', handleStorageEvent) | |
| if (timeoutId) clearTimeout(timeoutId) | |
| reject(new Error(`State mismatch: ${parsedResult.state} !== ${state}`)) | |
| } | |
| } | |
| } | |
| try { | |
| window.addEventListener('storage', handleStorageEvent) | |
| // Add timeout to prevent hanging promises | |
| timeoutId = setTimeout(() => { | |
| window.removeEventListener('storage', handleStorageEvent) | |
| reject(new Error('OAuth login timeout')) | |
| }, 300000) // 5 minutes | |
| } catch (error) { | |
| reject(error) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In app/core/social-handlers/EdlinkBaseHandler.js between lines 17 and 38, the
event listener for the 'storage' event is only removed when the promise resolves
successfully, which can cause memory leaks if the promise is rejected or an
error occurs before the event fires. To fix this, ensure the event listener is
removed in all cases where the promise settles, including rejection scenarios
and errors during setup. Add cleanup logic to remove the event listener both
when rejecting the promise and in the catch block to prevent memory leaks.
| const EdlinkBaseHandler = require('./EdlinkBaseHandler') | ||
|
|
||
| const PROVIDER_NAME = 'schoology' | ||
|
|
||
| module.exports = class SchoologyHandler extends CocoClass { | ||
| static async logInWithSchoology () { | ||
| const state = Math.random().toString(36).substring(2) | ||
| const url = `/auth/oauth2/${PROVIDER_NAME}?state=${state}` | ||
|
|
||
| window.open(url, '_blank', 'width=800,height=600', false) | ||
|
|
||
| const connectionTrackingKey = `${PROVIDER_NAME}ConnectionTrackingKey` | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| const handleStorageEvent = (event) => { | ||
| if (event.key === connectionTrackingKey) { | ||
| const result = localStorage.getItem(connectionTrackingKey) | ||
| const parsedResult = JSON.parse(result) | ||
| if (parsedResult.state === state) { | ||
| localStorage.removeItem(connectionTrackingKey) | ||
| window.removeEventListener('storage', handleStorageEvent) | ||
| const { state, ...rest } = parsedResult | ||
| resolve(rest) | ||
| } else { | ||
| reject(new Error(`State mismatch: ${parsedResult.state} !== ${state}`)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| window.addEventListener('storage', handleStorageEvent) | ||
| } catch (error) { | ||
| reject(error) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| async connect (options) { | ||
| this.trigger('connect') | ||
| const result = await SchoologyHandler.logInWithSchoology() | ||
|
|
||
| if (result.loggedIn) { | ||
| // login user if already signed up with Schoology | ||
| window.location.reload() | ||
| return | ||
| } | ||
|
|
||
| if (!result.email) { | ||
| throw new Error('No email found in Schoology response') | ||
| } | ||
|
|
||
| this.trigger('connect') | ||
| this.result = result | ||
| return options.success.bind(options.context)(result) | ||
| } | ||
|
|
||
| loadPerson (options) { | ||
| this.trigger('load-person', options) | ||
| return options.success.bind(options.context)(this.result) | ||
| module.exports = class SchoologyHandler extends EdlinkBaseHandler { | ||
| constructor () { | ||
| super('schoology') | ||
| } | ||
| } No newline at end of file | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Clean refactoring to use inheritance pattern.
The refactoring from a standalone OAuth2 implementation to extending EdlinkBaseHandler follows good software engineering practices and reduces code duplication.
Verify that the EdlinkBaseHandler class exists and provides the expected logInWithEdlink method:
🏁 Script executed:
#!/bin/bash
# Description: Verify EdlinkBaseHandler exists and has the expected method
# Expected: Should find the EdlinkBaseHandler class and logInWithEdlink method
# Check if EdlinkBaseHandler file exists
fd -t f "EdlinkBaseHandler.js"
# Look for the logInWithEdlink method in EdlinkBaseHandler
ast-grep --pattern $'class EdlinkBaseHandler {
$$$
logInWithEdlink($$$) {
$$$
}
$$$
}'Length of output: 176
Confirm EdlinkBaseHandler implementation
I located app/core/social-handlers/EdlinkBaseHandler.js, but it doesn’t define a logInWithEdlink method as expected. To complete this refactoring:
- Implement or rename the OAuth2 login helper in
EdlinkBaseHandler(e.g.logInWithEdlink(...)) so that subclasses can invoke it. - If the base class already provides equivalent functionality under a different name, update
SchoologyHandlerto call that method. - Add or update tests to cover the shared login flow in
EdlinkBaseHandler.
Please address these so that SchoologyHandler can rely on the central OAuth2 logic.
🤖 Prompt for AI Agents
In app/core/social-handlers/SchoologyHandler.js lines 1 to 7, the base class
EdlinkBaseHandler lacks the expected logInWithEdlink method needed for OAuth2
login. Review EdlinkBaseHandler.js to either implement this method or rename an
existing OAuth2 login helper to logInWithEdlink so subclasses like
SchoologyHandler can call it. Then update SchoologyHandler to invoke this
centralized login method. Finally, add or update tests in EdlinkBaseHandler to
cover the shared OAuth2 login flow.
| const SchoologyHandler = require('core/social-handlers/SchoologyHandler') | ||
| const ClassLinkHandler = require('core/social-handlers/ClassLinkHandler') |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Updated imports to support new handler architecture.
The import changes correctly reflect the refactoring from standalone functions to handler classes.
Verify that the ClassLinkHandler class exists:
🏁 Script executed:
#!/bin/bash
# Description: Verify ClassLinkHandler exists and follows the same pattern
# Expected: Should find the ClassLinkHandler class extending EdlinkBaseHandler
# Check if ClassLinkHandler file exists
fd -t f "ClassLinkHandler.js"
# Look for ClassLinkHandler class structure
ast-grep --pattern $'class ClassLinkHandler extends EdlinkBaseHandler {
$$$
}'Length of output: 156
🏁 Script executed:
#!/bin/bash
# Verify ClassLinkHandler class declaration and its base class
rg "class ClassLinkHandler" -n app/core/social-handlers/ClassLinkHandler.js
rg "extends" -n app/core/social-handlers/ClassLinkHandler.jsLength of output: 278
Validate ClassLinkHandler Import & Refactor Duplication
- Confirmed
ClassLinkHandlerexists atapp/core/social-handlers/ClassLinkHandler.jsand extendsEdlinkBaseHandler. - Imports in
app/views/core/AuthModal.js(lines 22–23) are correct. - Both login handlers in
AuthModal.js(Schoology: lines 323–330, ClassLink: lines 332–340) share identical success and error handling.
Consider extracting the shared login flow into a common helper to eliminate duplication.
🤖 Prompt for AI Agents
In app/views/core/AuthModal.js around lines 22-23 and 323-340, the login
handlers for Schoology and ClassLink share identical success and error handling
code. To fix this, create a common helper function that encapsulates the shared
login flow logic, then call this helper from both handlers to eliminate
duplication and improve maintainability.
| async onClickSchoologyLoginButton () { | ||
| const { loggedIn } = await logInWithSchoology() | ||
| const handler = new SchoologyHandler() | ||
| const { loggedIn } = await handler.logInWithEdlink() | ||
| if (loggedIn) { | ||
| window.location.reload() | ||
| } else { | ||
| noty({ text: $.i18n.t('login.schoology_login_failed'), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true }) | ||
| noty({ text: $.i18n.t('login.login_failed'), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true }) | ||
| } | ||
| } | ||
|
|
||
| async onClickClasslinkLoginButton () { | ||
| const handler = new ClassLinkHandler() | ||
| const { loggedIn } = await handler.logInWithEdlink() | ||
| if (loggedIn) { | ||
| window.location.reload() | ||
| } else { | ||
| noty({ text: $.i18n.t('login.login_failed'), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate code in login methods.
Both onClickSchoologyLoginButton and onClickClasslinkLoginButton methods have identical logic except for the handler class instantiation. This creates unnecessary code duplication.
Consider refactoring to a common method:
+ async logInWithEdlinkProvider(HandlerClass) {
+ const handler = new HandlerClass()
+ const { loggedIn } = await handler.logInWithEdlink()
+ if (loggedIn) {
+ window.location.reload()
+ } else {
+ noty({ text: $.i18n.t('login.login_failed'), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true })
+ }
+ }
async onClickSchoologyLoginButton () {
- const handler = new SchoologyHandler()
- const { loggedIn } = await handler.logInWithEdlink()
- if (loggedIn) {
- window.location.reload()
- } else {
- noty({ text: $.i18n.t('login.login_failed'), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true })
- }
+ return this.logInWithEdlinkProvider(SchoologyHandler)
}
async onClickClasslinkLoginButton () {
- const handler = new ClassLinkHandler()
- const { loggedIn } = await handler.logInWithEdlink()
- if (loggedIn) {
- window.location.reload()
- } else {
- noty({ text: $.i18n.t('login.login_failed'), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true })
- }
+ return this.logInWithEdlinkProvider(ClassLinkHandler)
}📝 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 onClickSchoologyLoginButton () { | |
| const { loggedIn } = await logInWithSchoology() | |
| const handler = new SchoologyHandler() | |
| const { loggedIn } = await handler.logInWithEdlink() | |
| if (loggedIn) { | |
| window.location.reload() | |
| } else { | |
| noty({ text: $.i18n.t('login.schoology_login_failed'), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true }) | |
| noty({ text: $.i18n.t('login.login_failed'), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true }) | |
| } | |
| } | |
| async onClickClasslinkLoginButton () { | |
| const handler = new ClassLinkHandler() | |
| const { loggedIn } = await handler.logInWithEdlink() | |
| if (loggedIn) { | |
| window.location.reload() | |
| } else { | |
| noty({ text: $.i18n.t('login.login_failed'), layout: 'topCenter', type: 'error', timeout: 5000, killer: false, dismissQueue: true }) | |
| } | |
| } | |
| // Extracted common login logic | |
| async logInWithEdlinkProvider(HandlerClass) { | |
| const handler = new HandlerClass() | |
| const { loggedIn } = await handler.logInWithEdlink() | |
| if (loggedIn) { | |
| window.location.reload() | |
| } else { | |
| noty({ | |
| text: $.i18n.t('login.login_failed'), | |
| layout: 'topCenter', | |
| type: 'error', | |
| timeout: 5000, | |
| killer: false, | |
| dismissQueue: true | |
| }) | |
| } | |
| } | |
| async onClickSchoologyLoginButton () { | |
| return this.logInWithEdlinkProvider(SchoologyHandler) | |
| } | |
| async onClickClasslinkLoginButton () { | |
| return this.logInWithEdlinkProvider(ClassLinkHandler) | |
| } |
🤖 Prompt for AI Agents
In app/views/core/AuthModal.js around lines 322 to 340, the
onClickSchoologyLoginButton and onClickClasslinkLoginButton methods contain
duplicated logic differing only by the handler class used. Refactor by creating
a single reusable async method that accepts the handler class as a parameter,
instantiates it, calls logInWithEdlink, and handles the login result. Then
update both button click methods to call this common method with the appropriate
handler class to eliminate duplication.
fixes eng-1833
Summary by CodeRabbit