sensei
sensei copied to clipboard
Automattic
WPML: Reusable student progress
Resolves #2788
Proposed Changes
Testing Instructions
New/Updated Hooks
Deprecated Code
Pre-Merge Checklist
- [ ] PR title and description contain sufficient detail and accurately describe the changes
- [ ] Acceptance criteria is met
- [ ] Decisions are publicly documented
- [ ] Adheres to coding standards (PHP, JavaScript, CSS, HTML)
- [ ] All strings are translatable (without concatenation, handles plurals)
- [ ] Follows our naming conventions (P6rkRX-4oA-p2)
- [ ] Hooks (p6rkRX-1uS-p2) and functions are documented
- [ ] New UIs are responsive and use a mobile-first approach
- [ ] New UIs match the designs
- [ ] Different user privileges (admin, teacher, subscriber) are tested as appropriate
- [ ] Legacy courses (course without blocks) are tested
- [ ] Code is tested on the minimum supported PHP and WordPress versions
- [ ] User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
- [ ] "Needs Documentation" label is added if this change requires updates to documentation
- [ ] Known issues are created as new GitHub issues
Codecov Report
Attention: Patch coverage is 71.28028% with 83 lines in your changes are missing coverage. Please review.
Project coverage is 51.85%. Comparing base (
7742f33) to head (5cc0fdc). Report is 4 commits behind head on trunk.
:exclamation: Current head 5cc0fdc differs from pull request most recent head c09d330. Consider uploading reports for the commit c09d330 to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## trunk #7492 +/- ##
============================================
+ Coverage 51.76% 51.85% +0.09%
- Complexity 11323 11337 +14
============================================
Files 641 646 +5
Lines 48185 48390 +205
Branches 470 470
============================================
+ Hits 24943 25095 +152
- Misses 22861 22914 +53
Partials 381 381
| Files | Coverage Δ | |
|---|---|---|
| includes/blocks/class-sensei-block-take-course.php | 78.37% <100.00%> (+0.29%) |
:arrow_up: |
| ...udes/blocks/class-sensei-course-progress-block.php | 100.00% <ø> (ø) |
|
| includes/class-sensei-course.php | 38.76% <100.00%> (+0.03%) |
:arrow_up: |
| includes/class-sensei-utils.php | 52.82% <100.00%> (+0.11%) |
:arrow_up: |
| .../class-sensei-course-manual-enrolment-provider.php | 91.66% <100.00%> (+0.23%) |
:arrow_up: |
| ...itories/class-comments-based-answer-repository.php | 100.00% <100.00%> (ø) |
|
| ...ositories/class-tables-based-answer-repository.php | 96.87% <100.00%> (+0.20%) |
:arrow_up: |
| ...sitories/class-comments-based-grade-repository.php | 100.00% <100.00%> (ø) |
|
| ...positories/class-tables-based-grade-repository.php | 96.96% <100.00%> (+0.12%) |
:arrow_up: |
| ...ies/class-comments-based-submission-repository.php | 100.00% <100.00%> (ø) |
|
| ... and 17 more |
... and 2 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 0132d5c...c09d330. Read the comment docs.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
WordPress Dependencies Report
The github-action-wordpress-dependencies-report action has detected some script changes between the commit c09d33011f550f68d07f63e854009ad3dbb12470 and trunk. Please review and confirm the following are correct before merging.
No changes detected in the current commit. But the comment was left so it is possible to check for the edit history.
This comment was automatically generated by the github-action-wordpress-dependencies-report action.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Create translations for each of them.
Should I be creating new lessons for the translated course via the course outline block, translating existing lessons individually, or both?
Do you know what happens in the case when the original course has more or less lessons than the translated course? Same thing for questions in a lesson.
Should I be creating new lessons for the translated course via the course outline block, translating existing lessons individually, or both?
@donnapep doesn't matter for this PR — it is simpler to translate the whole course, but if you want to do wider testing, you can try both.
Do you know what happens in the case when the original course has more or less lessons than the translated course? Same thing for questions in a lesson.
That was part of one of previous PRs: we check if we have translation and create a new one if it doesn't exist yet.
Should I be creating new lessons for the translated course via the course outline block, translating existing lessons individually, or both?
@donnapep doesn't matter for this PR — it is simpler to translate the whole course, but if you want to do wider testing, you can try both.
It doesn't seem like you can add translated lessons via the course outline block because then there's no way to associate the translated lesson with the original lesson, or is there some way I haven't found yet?
I spent a couple of hours setting up test data and familiarizing myself with WPML, but now whenever I browse to a translated course on the frontend (e.g. http://wpml.local/curso/how-to-play-the-piano-spanish/?lang=es), I get a 404. Do you know what the problem might be? I've already re-saved permalinks.
It doesn't seem like you can add translated lessons via the course outline block because then there's no way to associate the translated lesson with the original lesson, or is there some way I haven't found yet?
I think it is supposed that you maintain the same structure for both versions of the course.
In current implementation in WPML (and in our supporting logic), if you make changes in translations they are not synced with the original post. In fact, when you start editing the translated post WPML warns you that this translation is a stand-alone translation/post since then (can't find the exact message).
I spent a couple of hours setting up test data and familiarizing myself with WPML, but now whenever I browse to a translated course on the frontend (e.g. http://wpml.local/curso/how-to-play-the-piano-spanish/?lang=es), I get a 404. Do you know what the problem might be? I've already re-saved permalinks.
Oh, I'm afraid I have an idea... Might be related to the issue we had with Sensei slugs after changing the site language. It fixes there with re-saving permalinks, but for a multi-language website it might be a problem as two (or more) slugs need co-exist.
I test everything with Russian, it doesn't have custom slug. So I'll do some tests with Spanish.
@donnapep I started testing it with the third language (Spanish in this case).
It confirmed the issue you had with the slug. Basically, that's exactly the same issue we have when just change the language of the website (in this case we just flush rewrite rules). I don't have ideas how to handle co-existing slugs for the same entity properly. I'll check how Woo handles this case.
And it revealed a new issue with translation creation: in case there are translations for one direction (English-Russian, for example) it doesn't create a new direction (English-Spanish). I will work on it in a separate PR.
It confirmed the issue you had with the slug. Basically, that's exactly the same issue we have when just change the language of the website (in this case we just flush rewrite rules). I don't have ideas how to handle co-existing slugs for the same entity properly. I'll check how Woo handles this case.
Ah, so I'll bet this is the error that seems to still be an issue for our users. In the meantime, I've deleted the Spanish translation files so I can continue testing.
Here's some further testing feedback, some of which may not necessarily be related to this PR. Feel free to open separate issues as needed.
- I took a quiz in the translated course and chose an answer. When I clicked the Save Progress button, I got a fatal error:
PHP Fatal error: Uncaught RuntimeException: Missing lesson status. in /Users/donnapep/Local Sites/wpml/app/public/wp-content/plugins/sensei/includes/internal/quiz-submission/submission/repositories/class-comments-based-submission-repository.php:58
This was my quiz:
- For the same quiz, when I selected an answer and clicked Complete Quiz, nothing happened. The quiz reverted to its initial state with no answer selected.
- When I clicked the Complete Lesson button, the lesson status changed to Completed on the Students page, but there was nothing in the Date Completed column. This seemed to be a sporadic issue. Some lessons had a completed date but one didn't. I'm honestly not sure I could reproduce it again, but still thought it worth documenting.
I then reset the student's progress, and re-enrolled them to test again. This time, when I took the quiz I didn't see anything at all on the frontend:
Although it's there in the editor:
At this point I decided to stop testing quizzes and focused on course and lesson progress instead. 😅 So I removed all quizzes from the original lessons and translated lessons for a different course. Although everything looked good in the editor, I noticed that questions still retained their reference to the lesson:
I then created a brand new course without quizzes from the very start and didn't encounter any issues with course or lesson progress when taking either the original or the translated course.
I think the main concern from my testing is the unpredictable behavior when quizzes are involved. Do any of the issues I documented here seem to be related to the known issues we currently have?
@donnapep Thanks for testing it!
I'll check it thoroughly a bit later.
But I can say right now that previous experiments and tests might affect the result. The process is brittle 😢 I hope we'll be able to achieve a more stable state having more testing and feedback.
I continued testing (this time using WPML to translate) and have found an issue that has blocked me from further testing - I'm not seeing the Take Quiz button on a translated lesson, only the Complete Lesson button. The lesson has a quiz, and I've translated the strings, but there is no button to take the quiz and no Quiz link in the sidebar. Only the original language shows the button and links:
Frontend
Editor
I followed the instructions documented here and things went much more smoothly. 🎉
There are a couple of small things I noticed that I'm not sure are related to this PR, so I'd like to dig a bit deeper, but they shouldn't be considered blockers for this PR. I will open separate issues if they end up being legitimate bugs. I did also notice what you were saying about incorrect feedback being displayed even though I answered the question correctly. Just checking that we have an open issue for that one?
I haven't done a code review, but I think as soon as we have that, we can approve this one and get it merged.
@donnapep Yay! Yes, both issues with grading are logged here: https://github.com/orgs/Automattic/projects/404/views/51 (ToDo column).
I'm trying to test the quiz part using the branch fix/wpml-translate-slugs as you've suggested. For some reason, I could translate Course and Lesson, but getting 404 only for quizzes. I'm attaching videos in case it helps. I've also tried checking the "Don’t translate Sensei slugs" checkbox in Sensei LMS -> Settings -> WPML. But didn't help
Course and Lessons in backend:
https://github.com/Automattic/sensei/assets/6820724/8634cc23-6369-4373-8fd2-afa97f3fe4cf
Quiz in Frontend:
https://github.com/Automattic/sensei/assets/6820724/bde63228-c98e-4382-ba3b-77d5d7d11780
Ah okay, I added a Quiz for the second lesson and this time the URL is working. So maybe I made a mistake following the step. But don't know why the answers are appearing empty in the translated lesson -
https://github.com/Automattic/sensei/assets/6820724/f9c915d7-60f9-42a5-8ff9-eaee85a5daeb
Just wanted to document what I am seeing. I'll keep testing by putting values manually in those empty answer fields and see how it works going forward
I was just testing for paid courses, the purchase button disappears when I switch language -
https://github.com/Automattic/sensei/assets/6820724/2da0642e-a33b-4f5f-b19d-b67dfc4ffa78