Yuqiang/parents page dynamic corrily price #7735
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve modifications to the routing configuration, component imports, localization entries, and the User model. Specifically, the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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: 5
🧹 Outside diff range and nitpick comments (1)
app/models/User.js (1)
1116-1120: LGTM! Consider adding a comment for clarity.The addition of the
forcedValueparameter tostartHomeControlExperimentis a good improvement, allowing for more flexible control of the experiment value. This change aligns well with the PR objectives related to dynamic pricing on the parents page.Consider adding a brief comment explaining the purpose of the
forcedValueparameter for future maintainers. For example:startHomeControlExperiment (forcedValue) { + // forcedValue: Optional parameter to manually set the experiment value for testing or specific scenarios return this.getFilteredExperimentValue({ experimentName: 'home-page-filtered-control-experiment', forcedValue, }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- app/core/Router.js (0 hunks)
- app/core/vueRouter.js (1 hunks)
- app/locale/en.js (1 hunks)
- app/models/User.js (1 hunks)
- app/views/landing-pages/parents-v2/FeaturesGrid.vue (2 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- app/core/Router.js
🔇 Additional comments (10)
app/core/vueRouter.js (1)
52-53: Verify alignment with Corrily price testing objectiveThe component import for the
/parentsroute has been updated to always use the 'parents-v2' version, removing the previous A/B testing logic. While this simplifies the code, please clarify:
- How does this change support the PR objective of displaying a dynamic Corrily price for the self-paced annual subscription?
- Is the 'parents-v2' component configured to handle the Corrily price experimentation mentioned in the linked issue ENG-1213?
- Have we ensured that removing the A/B testing logic doesn't impact our ability to conduct the planned price experimentation in the US market?
To confirm the implementation of Corrily pricing in the new component, please run:
The simplification of the import logic is a positive change for code maintainability.
app/models/User.js (1)
Line range hint
1-1120: Confirm the removal ofgetParentsPageExperimentValueThe removal of the
getParentsPageExperimentValuemethod, as mentioned in the AI-generated summary, appears to be in line with the changes made tostartHomeControlExperiment. This suggests a shift in how experiments are managed for the parents page, which aligns with the PR objectives of updating the parents page for dynamic Corrily pricing.To ensure that the removal of
getParentsPageExperimentValuedoesn't have unintended consequences, please run the following script to check for any remaining references to this method:If the script returns any results, please review those occurrences and update them accordingly.
app/locale/en.js (6)
Line range hint
1-5957: Comprehensive update to localization stringsThis update includes extensive additions and modifications to the English localization strings. The changes cover various new features and sections of the application, including:
- A new 'parents_v2' section with detailed information about coding classes and curriculum
- A new 'roblox' section related to CodeCombat's integration with Roblox
- A new 'pd_page' section for professional development offerings
- A new 'junior_page' section for the CodeCombat Junior product
These additions significantly expand the content and features available in the application, providing more comprehensive information for parents, students, and educators.
Line range hint
5240-5715: New 'parents_v2' section addedA comprehensive 'parents_v2' section has been added, which includes:
- Testimonials from parents and students
- Detailed information about class packages and features
- Curriculum overview and concepts covered
- Instructor profiles
- Extensive FAQ section
This addition provides a wealth of information for parents considering CodeCombat's online coding classes for their children. It covers various aspects of the program, from pricing to curriculum details, making it easier for parents to make informed decisions.
Line range hint
5716-5775: New 'roblox' section for Roblox integrationA new 'roblox' section has been added, which includes:
- Information about linking CodeCombat and Roblox accounts
- Details on rewards for account linking
- Descriptions of Play, Code, and Create features within Roblox
- FAQs specific to the Roblox integration
This addition introduces CodeCombat's integration with Roblox, allowing users to extend their coding experience into the Roblox platform. It provides clear information about the benefits of linking accounts and what users can expect from this integration.
Line range hint
5776-5806: New 'pd_page' section for professional developmentA new 'pd_page' section has been added, which includes:
- Information about implementation training for educators
- Details on a 40+ hour professional development course
- Information about AP CSP professional development
- Links to download syllabi and course content
This addition provides comprehensive information about CodeCombat's professional development offerings for educators. It covers both implementation training and in-depth courses, including AP CSP preparation, which can help teachers effectively use CodeCombat and Ozaria in their classrooms.
Line range hint
5807-5957: New 'junior_page' section for CodeCombat JuniorA new 'junior_page' section has been added, which includes:
- Information about CodeCombat Junior, a K-5 curriculum
- Details on the scaffolded approach and accessibility features
- Information about adaptive learning and cross-curricular connections
- Links to resources like scope and sequence
- FAQs and trend information related to K-5 coding education
This addition introduces CodeCombat Junior, a new offering targeted at elementary school students. It provides comprehensive information about the curriculum's features, benefits, and how it adapts to younger learners' needs. This section will be valuable for parents and educators interested in introducing coding to younger children.
Line range hint
1-5957: Overall update to localization stringsThis update to the English localization file (app/locale/en.js) includes significant additions and some modifications:
Four major new sections have been added:
- 'parents_v2': Comprehensive information about online coding classes
- 'roblox': Details about CodeCombat's integration with Roblox
- 'pd_page': Information about professional development offerings
- 'junior_page': Introduction to CodeCombat Junior for K-5 students
Minor updates and additions to existing sections throughout the file
These changes represent a substantial expansion of CodeCombat's offerings, including new features, partnerships, and educational resources. The additions cater to a wider audience, from young learners to educators seeking professional development.
app/views/landing-pages/parents-v2/FeaturesGrid.vue (2)
18-18: Approved import of Vuex helpersThe import of
mapActionsandmapGettersfrom Vuex is correct and aligns with their usage in the component.
45-45: Confirm the correctness of new content entryThe addition of the new row in
rowsfor 'prompts allowance' appears to be consistent with the existing data structure.
| this.rows[2].content[1] = this.price | ||
| }, |
There was a problem hiding this comment.
Handle asynchronous data loading for 'price' computation
In the mounted() hook, you're setting this.rows[2].content[1] = this.price. However, since this.price depends on data loaded asynchronously in the created() hook via this.loadProducts(), there's a risk that this.price may not be initialized when mounted() runs.
Consider updating rows[2].content[1] after the data is loaded by watching for changes to basicAnnualSubscriptionForCurrentUser or this.price. Here's how you can implement it:
watch: {
price(newVal) {
if (newVal) {
this.rows[2].content[1] = newVal
}
}
},Alternatively, you can move the assignment inside the promise resolution of loadProducts() if it returns a promise:
created() {
this.loadProducts().then(() => {
this.rows[2].content[1] = this.price
}).catch(e => {
console.error('Error loading products in parents-v2', e)
})
},| try { | ||
| this.loadProducts() | ||
| } catch (e) { | ||
| console.error('Error loading products in parents-v2', e) | ||
| } | ||
| }, |
There was a problem hiding this comment.
Handle potential asynchronous errors in 'loadProducts'
While you're catching errors in the created() hook, loadProducts() might be an asynchronous action. If loadProducts() returns a promise, the try-catch block won't catch asynchronous errors.
Consider using .catch() to handle errors from the promise:
created() {
this.loadProducts().catch(e => {
console.error('Error loading products in parents-v2', e)
})
},Or use an async function:
async created() {
try {
await this.loadProducts()
} catch (e) {
console.error('Error loading products in parents-v2', e)
}
},Note: If you opt for an async created() hook, ensure that your environment supports it.
| return $.i18n.t('parents_v2.grid_self_paced_year_price_without_currencya', { price: p.get('formattedAmmount') }) | ||
| } else { | ||
| return $.i18n.t('parents_v2.grid_self_paced_year_price', { price: origPrice }) |
There was a problem hiding this comment.
Verify the i18n key for price formatting
The translation key parents_v2.grid_self_paced_year_price_without_currencya might be incorrect due to the trailing 'a'. Ensure that this key exists in your localization files. If it's a typo, it should likely be parents_v2.grid_self_paced_year_price_without_currency.
Apply this diff to correct the translation key:
- return $.i18n.t('parents_v2.grid_self_paced_year_price_without_currencya', { price: p.get('formattedAmount') })
+ return $.i18n.t('parents_v2.grid_self_paced_year_price_without_currency', { price: p.get('formattedAmount') })📝 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 $.i18n.t('parents_v2.grid_self_paced_year_price_without_currencya', { price: p.get('formattedAmmount') }) | |
| } else { | |
| return $.i18n.t('parents_v2.grid_self_paced_year_price', { price: origPrice }) | |
| return $.i18n.t('parents_v2.grid_self_paced_year_price_without_currency', { price: p.get('formattedAmount') }) | |
| } else { | |
| return $.i18n.t('parents_v2.grid_self_paced_year_price', { price: origPrice }) |
| origPrice = p.priceStringNoSymbol() | ||
| if (origPrice % 1 === 0) { | ||
| origPrice = Math.floor(origPrice) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure 'origPrice' is a number before performing arithmetic operations
The method p.priceStringNoSymbol() may return a string. When performing arithmetic operations like modulus, it's important to ensure that origPrice is a number to avoid unexpected results.
Convert origPrice to a number:
if (p) {
- origPrice = p.priceStringNoSymbol()
+ origPrice = Number(p.priceStringNoSymbol())
if (origPrice % 1 === 0) {
origPrice = Math.floor(origPrice)
}
}📝 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.
| origPrice = p.priceStringNoSymbol() | |
| if (origPrice % 1 === 0) { | |
| origPrice = Math.floor(origPrice) | |
| } | |
| } | |
| if (p) { | |
| origPrice = Number(p.priceStringNoSymbol()) | |
| if (origPrice % 1 === 0) { | |
| origPrice = Math.floor(origPrice) | |
| } | |
| } |
| if (p?.get('formattedAmmount')) { | ||
| return $.i18n.t('parents_v2.grid_self_paced_year_price_without_currencya', { price: p.get('formattedAmmount') }) |
There was a problem hiding this comment.
Fix typo in 'formattedAmmount' property name
The property 'formattedAmmount' is misspelled. It should be 'formattedAmount' (with one 'm'). This typo could lead to undefined values when accessing the property.
Apply this diff to correct the typo:
- if (p?.get('formattedAmmount')) {
- return $.i18n.t('parents_v2.grid_self_paced_year_price_without_currencya', { price: p.get('formattedAmmount') })
+ if (p?.get('formattedAmount')) {
+ return $.i18n.t('parents_v2.grid_self_paced_year_price_without_currency', { price: p.get('formattedAmount') })📝 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.
| if (p?.get('formattedAmmount')) { | |
| return $.i18n.t('parents_v2.grid_self_paced_year_price_without_currencya', { price: p.get('formattedAmmount') }) | |
| if (p?.get('formattedAmount')) { | |
| return $.i18n.t('parents_v2.grid_self_paced_year_price_without_currency', { price: p.get('formattedAmount') }) |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
app/views/landing-pages/parents-v2/FeaturesGrid.vue (1)
Line range hint
18-97: Overall implementation looks good with room for minor improvementsThe changes successfully implement dynamic pricing for the parents page, aligning with the PR objectives. Here's a summary of the improvements and suggestions:
- The Vuex integration for dynamic pricing is well-implemented.
- Error handling for asynchronous operations can be improved.
- There's a potential race condition in the
mountedhook that should be addressed.- The price computation logic can be simplified and made more robust.
- The changes to AI hints and prompts allowances need clarification.
To further improve the implementation:
- Consider adding unit tests for the
pricecomputed property to ensure it handles all scenarios correctly.- Implement error boundaries or fallback UI for cases where price loading fails.
- Add comments explaining the complex price calculation logic for better maintainability.
These improvements will enhance the reliability and maintainability of the dynamic pricing feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/views/landing-pages/parents-v2/FeaturesGrid.vue (2 hunks)
🔇 Additional comments (4)
app/views/landing-pages/parents-v2/FeaturesGrid.vue (4)
18-18: LGTM: Vuex imports addedThe addition of
mapActionsandmapGettersfrom Vuex is appropriate for implementing the dynamic pricing functionality.
45-46: Verify the purpose of AI hints and prompts allowance changesThe updates to AI hints (from 5 to 10) and prompts (from 100 to 50) allowances for the self-paced tier seem unrelated to the dynamic pricing implementation. Could you please clarify the reason for these changes?
67-74: LGTM: Price watcher implemented correctlyThe watcher for the 'price' property is well-implemented. The use of
immediate: trueensures that the rows are updated as soon as the component is created, which is good for initial rendering.
85-97: LGTM: Methods implemented correctlyThe
mapActionsandupdatePriceInRowsmethod are well-implemented. TheupdatePriceInRowsmethod efficiently updates the price in the rows data structure.
| computed: { | ||
| ...mapGetters('products', ['basicAnnualSubscriptionForCurrentUser']), | ||
| price () { | ||
| const p = this.basicAnnualSubscriptionForCurrentUser | ||
| let origPrice = 99 | ||
| if (p) { | ||
| origPrice = (p.amount / 100).toFixed(2) | ||
| if (origPrice % 1 === 0) { | ||
| origPrice = Math.floor(origPrice) | ||
| } | ||
| } | ||
| // we don't have coupon ID in parent page so no sale price here | ||
| if (p?.formattedAmmount) { | ||
| return $.i18n.t('parents_v2.grid_self_paced_year_price_without_currencya', { price: p.formattedAmmount }) | ||
| } else { | ||
| return $.i18n.t('parents_v2.grid_self_paced_year_price', { price: origPrice }) | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Refine price computation logic
The price computation looks good overall, but there are a few points to address:
- There's a typo in
formattedAmmount. It should beformattedAmount. - The decimal handling logic can be simplified.
- Consider using a more precise method for rounding.
Here's a suggested refactor:
computed: {
...mapGetters('products', ['basicAnnualSubscriptionForCurrentUser']),
price() {
const p = this.basicAnnualSubscriptionForCurrentUser
if (p?.formattedAmount) {
return $.i18n.t('parents_v2.grid_self_paced_year_price_without_currency', { price: p.formattedAmount })
} else if (p) {
const origPrice = (p.amount / 100).toFixed(2)
return $.i18n.t('parents_v2.grid_self_paced_year_price', { price: Number(origPrice) })
}
return $.i18n.t('parents_v2.grid_self_paced_year_price', { price: 99 })
},
},This refactor addresses the typo, simplifies the logic, and uses Number() for more precise rounding.
| created () { | ||
| try { | ||
| this.loadProducts() | ||
| } catch (e) { | ||
| console.error('Error loading products in parents-v2', e) | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in created hook
The error handling in the created hook is good, but it can be improved to handle asynchronous errors more effectively.
Consider using an async function or attaching a .catch() to the loadProducts call:
created() {
this.loadProducts().catch(e => {
console.error('Error loading products in parents-v2', e)
})
},This ensures that any asynchronous errors from loadProducts are properly caught and logged.
| mounted () { | ||
| this.rows[2].content[1] = this.price | ||
| }, |
There was a problem hiding this comment.
Address potential race condition in mounted hook
The mounted hook updates the price in the rows, but there's a potential race condition if the price isn't loaded when mounted runs.
Consider moving this logic to the price watcher or the updatePriceInRows method:
watch: {
price: {
immediate: true,
handler(newPrice) {
this.updatePriceInRows(newPrice)
},
},
},
methods: {
updatePriceInRows(newPrice) {
this.rows = this.rows.map(row => {
if (row.type === 'header') {
return { ...row, content<
6D4E
/span>: [row.content[0], newPrice, row.content[2], row.content[3]] }
}
return row
})
},
},This ensures that the rows are updated whenever the price changes, avoiding any race conditions.
fix ENG-1213
i cannot test it since i'm not in corrily price 😂 but I think I follow the subscribe modal logic. anyone can test it ?
Summary by CodeRabbit
New Features
'parents-v2'entry./parentsto directly import theparents-v2version of thePageParentscomponent.FeaturesGridcomponent to dynamically display subscription-based pricing.Bug Fixes