Send owner user email to self-service migrations #22759
Conversation
WalkthroughThis pull request updates the owner user retrieval mechanism within the admin migration flow. A new asynchronous method Possibly related PRs
Suggested reviewers
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. 🔧 ESLint
ghost/admin/app/components/gh-migrate-iframe.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
ghost/admin/app/services/migrate.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
✨ 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:
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
🔭 Outside diff range comments (1)
ghost/admin/app/components/gh-migrate-iframe.js (1)
58-70: 🛠️ Refactor suggestionAdd null check for owner user before accessing email property
The code assumes
theOwnerwill always be defined and have an email property. If no owner user is found by theownerUser()method, accessingtheOwner.emailwill throw an error.Consider adding a null check before accessing the email property:
async _handleUrlRequest() { const theToken = await this.migrate.apiToken(); const theOwner = await this.migrate.ownerUser(); this.migrate.getMigrateIframe().contentWindow.postMessage({ request: 'initialData', response: { apiUrl: this.migrate.apiUrl, apiToken: theToken, darkMode: this.feature.nightShift, stripe: this.migrate.isStripeConnected, ghostVersion: this.migrate.ghostVersion, - ownerEmail: theOwner.email + ownerEmail: theOwner?.email } }, new URL(this.migrate.getIframeURL()).origin); }
🧹 Nitpick comments (1)
ghost/admin/app/services/migrate.js (1)
67-78: Add error handling and JSDoc documentation to ownerUser methodThe method has good logic but lacks error handling for API calls and documentation.
Consider enhancing the method with:
- Error handling for the API call
- JSDoc documentation to clarify the return type (especially that it could be undefined)
+ /** + * Fetches the owner user of the Ghost instance + * @returns {Promise<Object|undefined>} The owner user object or undefined if not found + */ async ownerUser() { // Try to receive the owner user from the store let user = this.store.peekAll('user').findBy('isOwnerOnly', true); if (!user) { // load it when it's not there yet - await this.store.findAll('user', {reload: true}); - user = this.store.peekAll('user').findBy('isOwnerOnly', true); + try { + await this.store.findAll('user', {reload: true}); + user = this.store.peekAll('user').findBy('isOwnerOnly', true); + } catch (error) { + console.error('Failed to load users:', error); + // Optionally re-throw or handle specifically + } } return user; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/app/components/gh-migrate-iframe.js(2 hunks)ghost/admin/app/services/migrate.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (1)
ghost/admin/app/services/migrate.js (1)
11-11: LGTM!The store service injection is correctly added and necessary for the
ownerUser()method.
ErisDS
left a comment
ErisDS
left a comment
There was a problem hiding this comment.
Ideally, there'd be at least one test added for this change to prove it works and we get an email address
no ref - Sends the owner email address to the self-service migration tool, so that we can ensure all users have a great experience of migrating
no ref