[IMP] Slides: Add elearning feature to slides #29876
Conversation
qmo-odoo
force-pushed
the
master-slides-becomes-courses-qmo
branch
from
410c4c4
to
958af36
Compare
qmo-odoo
force-pushed
the
master-slides-becomes-courses-qmo
branch
2 times, most recently
from
6ef57c0
to
f32b090
Compare
There was a problem hiding this comment.
First pass of review. I have some questions and remarks.
This is a good start, but I think we need to clean a bit before going further.
-
MODELS
1/ I thought a course was a slide. What it the purpose of slide.course then ?
2/ what is the role of slide.resource ? It does not seem used. Moreover, if it contains a link to the slide, we should give it a more explicit name -
CONTROLLERS
Simply global comment before examinating the methods.
Try to namespace all the route. As we are in slides module, the root NS should be /slides/. Then partition it with /slide, /course, /section, ... So, you should have something like that:
HTTP pages
/slides/<object>
/slides/slide/<object>
/slides/course/<object>
HTTP action routes
/slides/slide/<object>/<action>
/slides/course/<object>/<action>
/slides/section/<object>/<action>
JSON action routes
/slides/slide/<action>
/slides/course/<action>
/slides/section/<action>
To avoid breaking the existing, do not rename badly named existing routes. Simply modify the controller method.
-
VIEWS
Some remarks on the xml id. Pay attention to the indentation in your xml files. -
JS
not reviewed yet. But don't forget at the end to remove all your console.log() ;)
| @@ -50,6 +62,16 @@ def _compute_promoted_slide_id(self): | |||
| limit=1, order=self._order_by_strategy[record.promote_strategy]) | |||
| record.promoted_slide_id = slides and slides[0] or False | |||
|
|
|||
| course_content_infos = fields.Char('Content of the course', compute="_compute_course_content_infos") | |||
There was a problem hiding this comment.
Try to put all field definition, then the computed method, but not field definition, compute method, second field definition, ... to be guidelines compliant.
|
|
||
| @http.route('/slide/setstate', website=True, type="json", auth="user") | ||
| def set_status_as_done(self, **kw): | ||
| slide = request.env['slide.slide'].browse(kw['slide']) |
There was a problem hiding this comment.
Make explicit and correclty named parameters: slide_id, user_id. Using only **kwargs allows human to call your controller without required parameters.
| def update_course(self, **kw): | ||
| course = request.env['slide.channel'].browse(kw['channel_id']) | ||
| kw.pop('channel_id') | ||
| course.write(kw) |
There was a problem hiding this comment.
Maybe we should restrict the fields you can write via this controller. Otherwise, you can write all you want here.
If so, then maybe use directly the write method might be a good idea.
qmo-odoo
force-pushed
the
master-slides-becomes-courses-qmo
branch
from
f32b090
to
54d3696
Compare
| else: | ||
| course.course_content_infos = " %s videos" % (course.nbr_videos) | ||
|
|
||
| total = fields.Integer(compute='_count_presentations', store=True) |
There was a problem hiding this comment.
What is this field ? total is not very precise IMHO.
Also, call the compute method _compute_<field name> ;)
| @@ -28,6 +34,23 @@ class SlideUsersRelation(models.Model): | |||
| user_id = fields.Many2one('res.users', index=True, required=True) | |||
| vote = fields.Integer('Vote', default=0) | |||
|
|
|||
| is_completed = fields.Boolean('Is completed',required=True) | |||
|
|
|||
|
|
||
| #Quiz related fields | ||
| question_ids = fields.One2many("slide.question","slide_id", string="Questions") | ||
| quiz_first_attempt_reward = fields.Integer(default=10,string="First Attempt") |
There was a problem hiding this comment.
string attributes first (without string=), then the others. Space after coma.
Tip: use a linter ;)
There was a problem hiding this comment.
Alsi "First Attempt" does not really match the field name "_reward".
Maybe "First Attempt Reward" ?
Same for other fields.
| if answer.is_correct: | ||
| good_answer_count += 1 | ||
| if good_answer_count > 1: | ||
| raise models.ValidationError('A question can only have one good answer') |
There was a problem hiding this comment.
from odoo.exceptions import ValidationError in the import list, as the error classes are not defined in models.
| 'is_completed': not bad_answers | ||
| }) | ||
| return { | ||
| 'goodAnswers':[good_answer.id for good_answer in good_answers], |
There was a problem hiding this comment.
good_answer.ids will do the job ;)
| points = slide[possible_points[user_slide_relation.quiz_attempts_count]] | ||
| else: | ||
| points = slide.quiz_fourth_attempt_reward | ||
| user_slide_relation.sudo().write({ |
There was a problem hiding this comment.
no need to sudo as the search is already done in sudo
| }) | ||
| return { | ||
| 'goodAnswers':[good_answer.id for good_answer in good_answers], | ||
| 'badAnswers': [bad_answer.id for bad_answer in bad_answers], |
There was a problem hiding this comment.
underscore notation please ;)
| @http.route('/slide/setstate', website=True, type="json", auth="user") | ||
| def set_status_as_done(self, **kw): | ||
| slide = request.env['slide.slide'].browse(kw['slide']) | ||
| user = request.env['res.users'].browse(kw['user']) |
There was a problem hiding this comment.
Any user can call this controller and set the relation slide-user to complete to every other user. We have a security issue here. Avoid giving the user and take the current one maybe.
| 'user': request.env.user, | ||
| 'is_public_user': request.env.user == request.website.user_id, | ||
| }) | ||
| else: |
…to gamification Purpose of this commit is to prepare addition of gamification in slides / eLearning platform. In order to be able to use karma and the badges in other modules we move those models in gamification. Partial commit linked to eLearning project. Main specifications related to gamification and user profile can be found on task 1922159 (PR odoo#30514). Main specifications related to eLearning can be found on task 1902304 (PR odoo#29876).
To encourage forum and slides users to be more active ranks are now added. They are directly linked to karma. The more the user has karma the more his rank will be high. The default rank is Newbie, with 1 point of karma. Users with 0 karma are considered as inactive on forum or slides. When a user reach a new rank a mail is sent to him to congratulate him with his new rank. To add a button in the mail template to allow users to go directly on a website section (like forum or slides) simply override get_gamification_redirection_data to add the target url. Partial commit linked to eLearning project. Main specifications related to gamification and user profile can be found on task 1922159 (PR odoo#30514). Main specifications related to eLearning can be found on task 1902304 (PR odoo#29876).
…mification Purpose is to be able to re-use frontend-related model, data and tools for gamification. Currently only forum uses gamification. Soon slides module will use them and user profile / biography will be expanded. This commit prepares those features by already preparing the bridge module and adding a publish option on badges. Partial commit linked to eLearning project. Main specifications related to gamification and user profile can be found on task 1922159 (PR odoo#30514). Main specifications related to eLearning can be found on task 1902304 (PR odoo#29876). Co-Authored-By: David Beguin <dbe@odoo.com> Co-Authored-By: Thibault Delavallee <tde@odoo.com>
Purpose is to prepare the use of gamification tools in slides. In this commit we add the dependency on website_profile (gamification front-end) in slides and add the slides category on challenges. Partial commit linked to eLearning project. Main specifications related to gamification and user profile can be found on task 1922159 (PR odoo#30514). Main specifications related to eLearning can be found on task 1902304 (PR odoo#29876). Co-Authored-By: David Beguin <dbe@odoo.com> Co-Authored-By: Thibault Delavallee <tde@odoo.com>
qmo-odoo
force-pushed
the
master-slides-becomes-courses-qmo
branch
from
5335027
to
e3a7341
Compare
| @@ -0,0 +1,394 @@ | |||
| var onYouTubeIframeAPIReady; | |||
There was a problem hiding this comment.
Generic comment: Your JS methods lack docstring.
Widget are usually split between private and public methods and every method should have its own docstring with "@OverRide", "@Private", "@param", ...
You can check out basic_fields.js for a good example of what it should ideally look like.
You probably planned to do it when everything is working and it's fine. But I have noticed it's easier to do it while coding the methods (or at least regularly) when you have a huge amount of JS (and it seems to be the case here).
There was a problem hiding this comment.
Also I'm not sure that your JS linter is correctly configured.
Here is my config for eslint just in case (using VS Code):
{ "env": { "commonjs": true, "es6": true }, "extends": "eslint:recommended", "parserOptions": { "sourceType": "module" }, "rules": { "indent": [ "off", 4, { "SwitchCase": 1 } ], "linebreak-style": [ "error", "unix" ], "quotes": [ "off", "double" ], "semi": [ "error", "always" ], "comma-dangle": [ "off" ], "no-console": [ "warn", { "allow": ["warn", "error"] } ], "no-unused-vars": [ "warn", { "args": "none" } ], "no-empty": [ "warn" ], "eqeqeq": [ "error", "smart" ], "space-before-function-paren": [ "warn", {"anonymous": "always", "named": "never"} ], "keyword-spacing": [ "warn" ], "no-use-before-define": [ "warn", "nofunc" ] }, "globals": { "$": false, "jQuery": false, "_": false, "openerp": true, "odoo": true, "CKEDITOR": true, "google": false, "window": false, "setTimeout": false, "clearTimeout": false, "document": false, "console": false, "QUnit": false, "moment": false, "FileReader": false, "nv": false, "d3": false, "ace": false, "Option": false, "py": false, "XMLHttpRequest": false, "setInterval": false, "clearInterval": false, "Image": false, "jstz": false, "ZeroClipboard": false, "sessionStorage": false, "Node": false, "history": false, "gapi": false, "Event": false, "Gravitec": false, "navigator": false, "OneSignal": false } }
| }, | ||
| start: function(){ | ||
| this._super.apply(this, arguments); | ||
| var user_id = this.$el.attr('user_id'); |
There was a problem hiding this comment.
Should probably use camel case for JS variable names.
| firstScriptTag.parentNode.insertBefore(tag, firstScriptTag); | ||
|
|
||
|
|
||
| var VideoPlayer = Widget.extend({ |
There was a problem hiding this comment.
Should probably use camel case for JS variable names.
| }, | ||
| _getSlides: function(){ | ||
| var self = this; | ||
| var slides = $('.slide-list-cell'); |
There was a problem hiding this comment.
You can probably use $('.slide-list-cell').each(function() {}) here.
| self.slides.push({ | ||
| id: parseInt(slide.attr('slide_id'), 10), | ||
| name: slide.attr('slide_name'), | ||
| embed_code: slide.attr('slide_embed_code'), |
There was a problem hiding this comment.
Should use camel case for JS variables.
| }, | ||
| _onAnswerClick: function(ev){ | ||
| var self = this; | ||
| var target = $(ev.currentTarget) |
There was a problem hiding this comment.
A lot of missing trailing ; here
| var self = this; | ||
| switch(ev.key){ | ||
| case "ArrowRight": | ||
| self._goToNextSlide(); |
There was a problem hiding this comment.
Missing one level of indentation on this line and the break;
There was a problem hiding this comment.
Also, I know that pressing left/right arrow on YT forwards the video by 5 seconds.
It may collide with this? Or maybe not since the event is caught by the iframe?
| // $('quiz-answer').not('.quiz-good-answer,.quiz-bad-answer').addClass('quiz-ignored-answer'); | ||
| if(data.passed){ | ||
| self.slide.done = true; | ||
| $('#check-'+self.slide.id).removeClass('text-muted'); |
There was a problem hiding this comment.
Again you can chain the calls with one selector.
| } | ||
| }) | ||
| } else { | ||
| $('.quiz-questions').append($('<p class="quiz-danger text-danger mt-1">All questions must be answered !</p>')); |
There was a problem hiding this comment.
I think you need to translate this with _t()
| }) | ||
| } | ||
| for(var i = 0; i < answers.badAnswers.length; i++){ | ||
| $('#'+ answers.badAnswers[i]).removeClass('quiz-good-answer'); |
There was a problem hiding this comment.
$badAnswer = $('#'+ answers.badAnswers[i])
$badAnswer.removeClass('quiz-good-answer')
.addClass('quiz-bad-answer')
.unbind('click');
$badAnswer.find('.radio-box span').replaceWith($('<i class="fa fa-times "></i>'));
$badAnswer.find('.radio-box input').prop('checked', false);
Or something like that but you get the point.
…to gamification Purpose of this commit is to prepare addition of gamification in slides / eLearning platform. In order to be able to use karma and the badges in other modules we move those models in gamification. Partial commit linked to eLearning project. Main specifications related to gamification and user profile can be found on task 1922159 (PR #30514). Main specifications related to eLearning can be found on task 1902304 (PR #29876).
To encourage forum and slides users to be more active ranks are now added. They are directly linked to karma. The more the user has karma the more his rank will be high. The default rank is Newbie, with 1 point of karma. Users with 0 karma are considered as inactive on forum or slides. When a user reach a new rank a mail is sent to him to congratulate him with his new rank. To add a button in the mail template to allow users to go directly on a website section (like forum or slides) simply override get_gamification_redirection_data to add the target url. Partial commit linked to eLearning project. Main specifications related to gamification and user profile can be found on task 1922159 (PR #30514). Main specifications related to eLearning can be found on task 1902304 (PR #29876).
qmo-odoo
force-pushed
the
master-slides-becomes-courses-qmo
branch
from
6cfe905
to
290ee6c
Compare
qmo-odoo
force-pushed
the
master-slides-becomes-courses-qmo
branch
from
290ee6c
to
09658b2
Compare
qmo-odoo
force-pushed
the
master-slides-becomes-courses-qmo
branch
from
09658b2
to
9abcae3
Compare
|
@robodoo r+ |
tde-banana-odoo
force-pushed
the
master-slides-becomes-courses-qmo
branch
from
9abcae3
to
b19d907
Compare
|
@robodoo r+ |
Purpose of this commit is to introduce a new way of displaying and managing slides in eLearning module. Its purpose is to give a better experience to users when going through a course. Notably a fullscreen mode allows to take lessons one by one without going out of the elearning display. Integration of all slide types eases taking the course step by step. Quiz are introduced in this commit. Those are an addition to slides with some question / answers (multiple choice). It allows customer to gain karma and improves gamification. Certifications are still done using the survey application. Quiz targets only small tests at the end of a given slide. A new widget is added to display and control slide display in fullscreen mode. Old display is still available for documentation channels or when going out of fullscreen mode, to see details on comments / review / statistics and have access to share options. This commit is linked to task ID 1902304 and PR odoo#29876. It closes the main work on refactoring website slides into eLearning, with other tasks already merged in current community [1][2][3][4][5]. [1] Task ID 1940360 landed at b2149bc: certification inclusion [2] Task ID 1936153 landed at 6c6179e: homepage inclusion [3] Task ID 1937160 landed at 8b7605c: selling courses [4] Task ID 1922159 and 1940516 landed at 5fa651c and 6a64c3c: new user profile and gamification of slides / forum [5] Task ID 1938643 landed at 77b5673: upload channel/slide
tde-banana-odoo
force-pushed
the
master-slides-becomes-courses-qmo
branch
from
b19d907
to
a50600d
Compare
|
@robodoo r+ |
Purpose of this commit is to introduce a new way of displaying and managing slides in eLearning module. Its purpose is to give a better experience to users when going through a course. Notably a fullscreen mode allows to take lessons one by one without going out of the elearning display. Integration of all slide types eases taking the course step by step. Quiz are introduced in this commit. Those are an addition to slides with some question / answers (multiple choice). It allows customer to gain karma and improves gamification. Certifications are still done using the survey application. Quiz targets only small tests at the end of a given slide. A new widget is added to display and control slide display in fullscreen mode. Old display is still available for documentation channels or when going out of fullscreen mode, to see details on comments / review / statistics and have access to share options. This commit is linked to task ID 1902304 and PR #29876. It closes the main work on refactoring website slides into eLearning, with other tasks already merged in current community [1][2][3][4][5]. [1] Task ID 1940360 landed at b2149bc: certification inclusion [2] Task ID 1936153 landed at 6c6179e: homepage inclusion [3] Task ID 1937160 landed at 8b7605c: selling courses [4] Task ID 1922159 and 1940516 landed at 5fa651c and 6a64c3c: new user profile and gamification of slides / forum [5] Task ID 1938643 landed at 77b5673: upload channel/slide
Purpose of this commit is to introduce a new way of displaying and managing
slides in eLearning module. Its purpose is to give a better experience to
users when going through a course. Notably a fullscreen mode allows to take
lessons one by one without going out of the elearning display. Integration
of all slide types eases taking the course step by step.
Quiz are introduced in this commit. Those are an addition to slides with
some question / answers (multiple choice). It allows customer to gain karma
and improves gamification. Certifications are still done using the survey
application. Quiz targets only small tests at the end of a given slide.
A new widget is added to display and control slide display in fullscreen
mode. Old display is still available for documentation channels or when
going out of fullscreen mode, to see details on comments / review / statistics
and have access to share options.
This commit is linked to task ID 1902304 and PR #29876. It closes the main
work on refactoring website slides into eLearning, with other tasks already
merged in current community [1][2][3][4][5].
[1] Task ID 1940360 landed at b2149bc: certification inclusion
[2] Task ID 1936153 landed at 6c6179e: homepage inclusion
[3] Task ID 1937160 landed at 8b7605c: selling courses
[4] Task ID 1922159 and 1940516 landed at 5fa651c and 6a64c3c:
new user profile and gamification of slides / forum
[5] Task ID 1938643 landed at 77b5673: upload channel/slide