Refactor Congratulations component and update SQL queries and localization support #1362
Conversation
…ter layout and text handling
WalkthroughThis pull request encompasses a diverse set of changes across multiple services and modules. The modifications include refactoring the Changes
Possibly related PRs
Suggested labels
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 (
|
github-actions
bot
added
shared-module
course-material
headless-lms
main-frontend
labels
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
shared-module/packages/common/src/components/Reference.tsx (1)
Line range hint
1-238: Consider modernizing the implementation for better maintainability and accessibility.While the component works, there are several areas where it could be improved:
Replace DOM manipulations with React patterns:
- Use refs instead of
document.querySelector- Manage tooltip state in React instead of direct DOM manipulation
- Handle styles through emotion instead of inline styles
Enhance accessibility:
- Add proper ARIA attributes to the details/summary elements
- Implement keyboard navigation for the tooltips
- Consider touch device support for the hover interactions
Optimize performance:
- Memoize event handlers
- Reduce DOM queries
- Consider using a virtual list for large reference lists
Here's a suggested approach for the tooltip implementation:
const ReferenceTooltip: React.FC<{text: string; isVisible: boolean; position: {x: number; y: number}}> = memo(({text, isVisible, position}) => { const tooltipStyles = css` opacity: ${isVisible ? 1 : 0}; position: absolute; top: ${position.y}px; left: ${position.x}px; // ... other styles `; return isVisible ? ( <div className={tooltipStyles} role="tooltip"> {text} </div> ) : null; });And for the main component:
const Reference: React.FC<ReferenceProps> = ({data}) => { const [activeTooltip, setActiveTooltip] = useState<{id: string; position: {x: number; y: number}} | null>(null); const detailsRef = useRef<HTMLDetailsElement>(null); const handleReferenceHover = useCallback((event: React.MouseEvent, citation: Reference) => { const target = event.currentTarget.getBoundingClientRect(); setActiveTooltip({ id: citation.id, position: { x: target.left + window.scrollX, y: target.bottom + window.scrollY } }); }, []); // ... rest of the implementation };Would you like me to provide more specific implementation details for any of these improvements?
shared-module/packages/common/src/locales/uk/course-material.json (1)
Line range hint
1-1: Missing marketing consent translations.The marketing consent related keys that are present in the Finnish localization file are missing from the Ukrainian translations:
- "marketing-consent-checkbox-text"
- "marketing-consent-privacy-policy-checkbox-text"
Add the following keys with their Ukrainian translations:
{ + "marketing-consent-checkbox-text": "Я погоджуюся отримувати оновлення про майбутні мовні версії та інформацію про нові курси. Я погоджуюся надати свою контактну інформацію для отримання персоналізованих повідомлень на сторонніх платформах. Дізнайтеся більше в нашій Політиці конфіденційності.", + "marketing-consent-privacy-policy-checkbox-text": "Я приймаю Політику конфіденційності та Умови надання послуг.", // existing translations... }
🧹 Nitpick comments (3)
services/main-frontend/src/components/forms/NewCourseForm.tsx (2)
173-173: Consider enhancing error handlingWhile the ErrorBanner implementation improves UI consistency, consider these enhancements:
- Format error messages for better user experience instead of using raw Error.toString()
- Clear errors when form fields change to provide immediate feedback
Example implementation:
const [error, setError] = useState<string | null>(null) +const formatErrorMessage = (error: Error): string => { + // Add custom error message mapping here + return error.message || t("generic-error-message") +} + +const clearError = () => setError(null) + // In your catch block: -setError(e.toString()) +setError(formatErrorMessage(e)) + // In your field change handlers: onChangeByValue={(value) => { setName(value) setSlug(normalizePath(value)) + clearError() }}
Line range hint
1-365: Consider architectural improvements for better maintainabilityThe form implementation could benefit from several architectural improvements:
Form Management:
- Consider using a form library like Formik or React Hook Form for more robust validation and state management
- This would eliminate the need for multiple useState calls and manual form validation
Business Logic Separation:
- Extract course creation and duplication logic into custom hooks
- Separate language code validation into a utility function
State Management:
- Consider using a reducer to manage related state variables
- This would make state transitions more predictable and easier to test
Here's a high-level example of how this could be structured:
// hooks/useNewCourseForm.ts interface NewCourseFormState { name: string; slug: string; languageCode: string; isDuplicate: boolean; // ... other form fields } interface NewCourseFormActions { setName: (name: string) => void; setLanguageCode: (code: string) => void; // ... other actions } const useNewCourseForm = (initialState: NewCourseFormState): [NewCourseFormState, NewCourseFormActions] => { const [state, dispatch] = useReducer(formReducer, initialState); // ... form logic return [state, actions]; } // utils/languageValidation.ts export const validateLanguageCode = (code: string): ValidationResult => { // ... validation logic } // NewCourseForm.tsx const NewCourseForm: React.FC<NewCourseFormProps> = (props) => { const [formState, formActions] = useNewCourseForm(initialState); const createCourseMutation = useCreateCourseMutation(); const duplicateCourseMutation = useDuplicateCourseMutation(); // ... render form }shared-module/packages/common/src/locales/ar/main-frontend.json (1)
Line range hint
1-1: Consider moving marketing-related translations to course-material.json.For better maintainability and consistency across languages, consider:
- Moving marketing-related translations to course-material.json
- Adding the full marketing consent text translations as seen in the Finnish version
Create a new
course-material.jsonfile for Arabic with the following translations:// ar/course-material.json { + "marketing-consent-checkbox-text": "أوافق على تلقي تحديثات حول الإصدارات اللغوية المستقبلية ومعلومات حول الدورات الجديدة. أوافق على مشاركة معلومات الاتصال الخاصة بي لتلقي رسائل مخصصة على المنصات الخارجية. اقرأ المزيد في سياسة الخصوصية الخاصة بنا.", + "marketing-consent-privacy-policy-checkbox-text": "أوافق على سياسة الخصوصية وشروط الخدمة." }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
services/course-material/src/components/ContentRenderer/moocfi/CongratulationsBlock/Congratulations.tsx(1 hunks)services/headless-lms/models/.sqlx/query-9b0e62cfe38673577b24eb3b1d2094f147a5ef7ba99446546bdfead686df2e88.json(1 hunks)services/headless-lms/models/.sqlx/query-c94f61a635ad062e122b9cf0ed8085858924769294d0c5d6df7b29634b8485d0.json(0 hunks)services/headless-lms/models/.sqlx/query-cdcc2e446166b920f1af4e384e2dc03e4478afb22198d49e7acbd393359fbaeb.json(1 hunks)services/headless-lms/models/src/library/copying.rs(2 hunks)services/main-frontend/src/components/forms/EditReferenceForm.tsx(0 hunks)services/main-frontend/src/components/forms/NewCourseForm.tsx(2 hunks)shared-module/packages/common/src/components/Reference.tsx(1 hunks)shared-module/packages/common/src/locales/ar/course-material.json(2 hunks)shared-module/packages/common/src/locales/ar/main-frontend.json(1 hunks)shared-module/packages/common/src/locales/en/course-material.json(1 hunks)shared-module/packages/common/src/locales/fi/course-material.json(2 hunks)shared-module/packages/common/src/locales/uk/course-material.json(1 hunks)shared-module/packages/common/src/locales/uk/main-frontend.json(1 hunks)
💤 Files with no reviewable changes (2)
- services/main-frontend/src/components/forms/EditReferenceForm.tsx
- services/headless-lms/models/.sqlx/query-c94f61a635ad062e122b9cf0ed8085858924769294d0c5d6df7b29634b8485d0.json
🔇 Additional comments (17)
shared-module/packages/common/src/components/Reference.tsx (1)
90-91: LGTM! Good handling of long content.
The addition of both word-wrap and overflow-wrap properties ensures proper text wrapping for long content across different browsers.
services/main-frontend/src/components/forms/NewCourseForm.tsx (1)
10-10: LGTM: Import statement is properly organized
The ErrorBanner import follows the existing pattern of importing shared components.
services/course-material/src/components/ContentRenderer/moocfi/CongratulationsBlock/Congratulations.tsx (2)
106-108: LGTM: Clear and descriptive variable name with correct logic.
The variable effectively checks both module completion and certification status.
110-120: LGTM: Well-structured conditional logic for determining subtitle text.
The function cleanly handles all possible combinations of certification and credit registration states, with proper i18n integration.
Also applies to: 128-128
shared-module/packages/common/src/locales/ar/course-material.json (2)
127-128: LGTM: Clear and consistent marketing consent messages in Arabic.
The translations maintain proper Arabic language conventions while accurately conveying the consent requirements.
215-218: LGTM: Well-structured course completion messages in Arabic.
The translations effectively communicate the different completion scenarios (basic completion, with credits, with certificate) while maintaining natural Arabic language flow.
shared-module/packages/common/src/locales/en/course-material.json (1)
215-218: LGTM: Clear and comprehensive completion messages.
The messages effectively communicate:
- Basic course completion
- Steps for certificate generation
- Process for ECTS credits
- Combined certificate and credits scenario
shared-module/packages/common/src/locales/fi/course-material.json (2)
129-130: LGTM! Marketing consent messages are well-structured.
The translations are clear and comprehensive, properly covering both marketing consent and privacy policy acceptance.
219-222: LGTM! Course completion messages cover all scenarios.
The translations effectively handle different completion scenarios:
- Basic course completion
- Certificate generation
- Credits registration
- Combined credits and certificate
shared-module/packages/common/src/locales/uk/course-material.json (1)
220-223: LGTM! Course completion messages are well translated.
The Ukrainian translations effectively cover all completion scenarios and maintain consistency with other language versions.
shared-module/packages/common/src/locales/ar/main-frontend.json (1)
303-303: LGTM! Marketing consent label is properly translated.
The Arabic translation for the marketing consent label is clear and accurate.
shared-module/packages/common/src/locales/uk/main-frontend.json (1)
304-304: LGTM! The new localization key is well-structured.
The added key "label-ask-for-marketing-consent" follows the established naming conventions and the Ukrainian translation "Запитати згоду на маркетинг" is grammatically correct.
services/headless-lms/models/src/library/copying.rs (3)
211-211: Invoke the new copy function in the right place
This call to copy_peer_or_self_review_questions is properly placed after copy_peer_or_self_review_configs to ensure prerequisite data is already copied. This sequencing helps avoid foreign key or orphaned data issues.
796-802: Small consistency check
The execution and immediate return flow here looks consistent. Just ensure that any cleanup or logging happens if an insertion fails.
783-795: Ensure correct use of LEFT JOIN
The code uses LEFT JOIN on exercises but also filters out deleted exercises in the WHERE clause with AND e.deleted_at IS NULL. As a result, rows without a matching exercise record will be excluded anyway, effectively making this an INNER JOIN. If the intention is to only copy configurations with valid exercises, an explicit INNER JOIN might be clearer.
✅ Verification successful
Based on the evidence gathered, I can now provide a final response:
LEFT JOIN is appropriate here - keep as is
The LEFT JOIN with the exercises table is actually the correct approach in this context. The table peer_or_self_review_configs was renamed from peer_review_configs as part of a migration that added self-review functionality. The LEFT JOIN combined with e.deleted_at IS NULL ensures that we only copy configurations that have valid, non-deleted exercises while maintaining data integrity. An INNER JOIN would be functionally equivalent here, but the LEFT JOIN makes it clearer that we're checking for the existence of valid exercises, which is a legitimate business requirement in this copying operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verifying if any peer_or_self_review_configs record has no matching exercises
# This script will list all such configurations if they exist
ast-grep --pattern $'SELECT posrc.id FROM peer_or_self_review_configs posrc LEFT JOIN exercises e ON (e.id = posrc.exercise_id)'
Length of output: 129
Script:
#!/bin/bash
# Let's search for SQL queries involving peer_or_self_review_configs and exercises tables
rg -U "peer_or_self_review_configs.*exercises" -A 5 -B 5
Length of output: 1905
Script:
#!/bin/bash
# Let's search for the SQL schema or table definitions to understand the relationship
rg -U "CREATE TABLE.*peer_or_self_review_configs" -A 10
rg -U "CREATE TABLE.*exercises" -A 10
Length of output: 2489
Script:
#!/bin/bash
# Let's search for all SQL queries involving peer_or_self_review_configs to understand the data integrity requirements
rg -U "peer_or_self_review_configs" -A 5 -B 5 "*.sql"
Length of output: 99
Script:
#!/bin/bash
# Let's search for migrations or schema files containing peer_or_self_review_configs
rg -U "peer_or_self_review_configs" -A 5 -B 5 --glob "*.sql"
# Also search for the table creation SQL
rg -U "CREATE TABLE.*peer_or_self_review_configs" -A 10 --glob "*.sql"
Length of output: 2777
services/headless-lms/models/.sqlx/query-cdcc2e446166b920f1af4e384e2dc03e4478afb22198d49e7acbd393359fbaeb.json (1)
1-12: Copy config query logic looks consistent
This insertion query properly excludes deleted records and exercises, promoting data integrity during copy. Consider confirming that any constraints (e.g., uniqueness, foreign keys) in the database schema won’t be inadvertently violated.
services/headless-lms/models/.sqlx/query-9b0e62cfe38673577b24eb3b1d2094f147a5ef7ba99446546bdfead686df2e88.json (1)
1-12: Query ensures only active questions are copied
The code looks correct and complements the config-copy logic by thoroughly filtering out deleted records. Ensure that the references to exercises and configs align with the newly inserted config IDs, preventing orphaned references.
| async fn copy_peer_or_self_review_questions( | ||
| tx: &mut PgConnection, | ||
| namespace_id: Uuid, | ||
| parent_id: Uuid, | ||
| ) -> ModelResult<()> { | ||
| sqlx::query!( | ||
| " | ||
| INSERT INTO peer_or_self_review_questions ( | ||
| id, | ||
| peer_or_self_review_config_id, | ||
| order_number, | ||
| question, | ||
| question_type, | ||
| answer_required, | ||
| weight | ||
| ) | ||
| SELECT uuid_generate_v5($1, q.id::text), | ||
| uuid_generate_v5($1, q.peer_or_self_review_config_id::text), | ||
| q.order_number, | ||
| q.question, | ||
| q.question_type, | ||
| q.answer_required, | ||
| q.weight | ||
| FROM peer_or_self_review_questions q | ||
| JOIN peer_or_self_review_configs posrc ON (posrc.id = q.peer_or_self_review_config_id) | ||
| JOIN exercises e ON (e.id = posrc.exercise_id) | ||
| WHERE peer_or_self_review_config_id = $2 | ||
| AND q.deleted_at IS NULL | ||
| AND e.deleted_at IS NULL | ||
| AND posrc.deleted_at IS NULL; |
There was a problem hiding this comment.
💡 Codebase verification
The WHERE clause in copy_peer_or_self_review_questions is incorrect and needs to be fixed
The current implementation has a mismatch between the copying of configs and questions:
copy_peer_or_self_review_configscopies all configs for a course (WHERE posrc.course_id = $2)- But
copy_peer_or_self_review_questionsonly copies questions for a single config (WHERE peer_or_self_review_config_id = $2)
This is incorrect because:
- The parent_id parameter passed to both functions is the course_id
- The questions function tries to use this course_id to match against config_id
- This will result in no questions being copied since a course_id will never match a config_id
The WHERE clause in copy_peer_or_self_review_questions should be changed to join with the course_id instead, similar to the configs query.
🔗 Analysis chain
Validate that only questions for a single config are copied
The WHERE clause checks peer_or_self_review_config_id = $2, targeting a single configuration ID. If there are multiple peer_or_self_review_configs for a course, this will not copy their associated questions. Confirm that this matches the intended behavior—copying exactly one config’s questions rather than all.
Let me gather more context about how the peer review configs are copied to understand the intended behavior better.
Let me try a different approach to find how the configs are copied using ripgrep instead.
Let me check the broader context of the copying functionality in this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checking references to copy_peer_or_self_review_questions calls to see if multiple configs are expected
rg "copy_peer_or_self_review_questions"
Length of output: 273
Script:
#!/bin/bash
# Check how peer_or_self_review_configs are copied
ast-grep --pattern 'INSERT INTO peer_or_self_review_configs'
Length of output: 62
Script:
#!/bin/bash
# Search for SQL queries related to peer_or_self_review_configs copying
rg -A 10 "peer_or_self_review_configs.*INSERT"
Length of output: 48
Script:
#!/bin/bash
# Look for the SQL query that copies peer_or_self_review_configs
rg -B 5 -A 15 "peer_or_self_review_configs" services/headless-lms/models/src/library/copying.rs
Length of output: 2127
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
services/main-frontend/src/components/forms/NewReferenceForm.tsx (5)
37-42: Suggest a naming refinement for clarity.Currently, the local variable references (line 41) has the same name as the "references" field in the form data, which might be confusing when browsing the code. Consider renaming one of them to avoid collision.
- const references = watch("references") + const watchedReferences = watch("references")
46-52: Validate naming consistency and data transformations.
- The
referencesvariable here may overshadow the previously declared "references" from watch.- If you anticipate large reference data, consider caching the BibTeX results or handling errors in a separate utility function to keep the component lightweight.
65-66: Confirm intended function name.The function is spelled “useCitataionLabelsThatWillChange.” This appears to be a typo of “useCitationLabelsThatWillChange.” Renaming ensures clarity in the codebase.
- const citationLabelsThatWillChange = useCitataionLabelsThatWillChange(references) + const citationLabelsThatWillChange = useCitationLabelsThatWillChange(references)
103-108: Consider adding error handling or returning fallback.While this function safely rebuilds the citation object, it doesn’t handle invalid references internally. If used elsewhere without a try/catch, it can throw runtime errors. Confirm that all external callers handle the potential failures, or handle them here to improve robustness.
110-123: Rename the function for correctness and clarity.“useCitataionLabelsThatWillChange” seems to have a typographical error. Original vs. safe label logic is beneficial, but the name should be updated to avoid confusion:
-export function useCitataionLabelsThatWillChange( +export function useCitationLabelsThatWillChange( references: string, ): { original: string; safe: string }[] { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
services/main-frontend/src/components/forms/EditReferenceForm.tsx(4 hunks)services/main-frontend/src/components/forms/NewReferenceForm.tsx(4 hunks)shared-module/packages/common/src/locales/ar/main-frontend.json(2 hunks)shared-module/packages/common/src/locales/en/main-frontend.json(1 hunks)shared-module/packages/common/src/locales/fi/main-frontend.json(1 hunks)shared-module/packages/common/src/locales/uk/main-frontend.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- services/main-frontend/src/components/forms/EditReferenceForm.tsx
- shared-module/packages/common/src/locales/uk/main-frontend.json
- shared-module/packages/common/src/locales/ar/main-frontend.json
🔇 Additional comments (5)
shared-module/packages/common/src/locales/en/main-frontend.json (1)
558-558: LGTM! Clear and well-structured error message.
The English translation is clear, grammatically correct, and effectively communicates:
- What will happen: "The label {{original}} will be changed {{safe}}"
- Why it happens: "because it contains unsafe characters"
shared-module/packages/common/src/locales/fi/main-frontend.json (1)
563-563: LGTM! Accurate and natural Finnish translation.
The Finnish translation:
- Maintains semantic equivalence with the English version
- Uses natural Finnish language constructs
- Correctly preserves the template variables
services/main-frontend/src/components/forms/NewReferenceForm.tsx (3)
88-92: Error label messages are properly handled.
The logic to display changed citation labels is clear, helps the user see what changed, and indicates potential issues. Good job!
125-133: Simple and effective reference validation.
The “areCitationsValid” function’s try/catch approach has good clarity: any parsing errors are gracefully handled by returning false. This is straightforward and maintains code clarity.
85-85: Verify that "autoResize" is supported.
Please confirm that your text area component library or the underlying implementation (e.g., a custom TextAreaField) supports the "autoResize" prop. If not, it’s best to remove or replace it with a supported property.
✅ Verification successful
The search results show that autoResize is a supported prop in the shared TextAreaField component located at shared-module/packages/common/src/components/InputFields/TextAreaField.tsx. The component properly implements the auto-resize functionality through a prop interface and handles the resizing logic in its implementation. The prop is also being used consistently across multiple components in the codebase.
The autoResize prop is properly supported by the TextAreaField component.
- The prop is defined in the component's interface:
autoResize?: boolean - The implementation includes proper height adjustment logic when
autoResizeis enabled - The prop is used consistently across multiple form components in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for 'autoResize' usage in the codebase to confirm it's supported by the relevant component.
rg -A 3 'autoResize'
Length of output: 7750
Summary by CodeRabbit
New Features
Localization Updates
Styling Improvements
Bug Fixes