Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MERGE][ADD] website_slides_survey: add a new bridge between slide and survey #31060

Closed

Conversation

awa-odoo
Copy link
Contributor

@awa-odoo awa-odoo commented Feb 13, 2019

Task #1940360
Subtask of #1902304

Purpose

Adds certification capabilities to the website_slides module.
Channel/courses can now include certifications as a new type of slide.

This new type of slide is available as a "Certification" button on the slide creation
frontend view (next to "Video", "Presentation", ...).
Users have to link the slide to an actual survey that has the 'certificate' field
set to true (that will populate the slide's survey_id field).

Slides of type certification are handled in frontend in a very simple way for now:

  • A button "Begin certification" that redirects the user to the related survey frontend.
  • A button "Download certification" when the user has succeeded the certification.
    (A new download route has been added in the survey module to make it work.)
  • When the survey is done, if it's linked to a slide, a button "Go back to course" allows
    the user to go back to the slide frontend.

(There is a special use case for when the website_publisher designing the survey lands on a
certification slide: he is allowed to test the survey with a survey_input created as test_entry)

Survey creation as well as limited time, limited number of attempts, ... are still completely
handled in the survey module. That means that the user will have to first create a suitable survey
that is a certification and only then create a slide of type "certification" and link
the created survey to it.

Ideally, the taking of the survey should be transparently included in the slide frontend but
it requires a full refactoring of the way surveys are submitted. This will most likely come in
a later commit.

This commit is an advanced merge of full eLearning module (see task #1902304).

@awa-odoo awa-odoo added the RD research & development, internal work label Feb 13, 2019
@awa-odoo awa-odoo force-pushed the master-slides-survey-bridge-awa branch 4 times, most recently from 0f708cd to 57fc0e9 Compare February 13, 2019 13:02
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Feb 13, 2019
@awa-odoo awa-odoo force-pushed the master-slides-survey-bridge-awa branch from 57fc0e9 to af894e5 Compare February 14, 2019 09:06
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Feb 14, 2019
@awa-odoo awa-odoo force-pushed the master-slides-survey-bridge-awa branch 3 times, most recently from aabb8c5 to b402da9 Compare February 14, 2019 10:49
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Feb 14, 2019
@awa-odoo awa-odoo force-pushed the master-slides-survey-bridge-awa branch from b402da9 to a996a22 Compare February 14, 2019 14:00
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Feb 14, 2019
@@ -68,11 +68,14 @@
</page>
<page string="Document">
<group>
<field name="url" attrs="{'required': [('image', '=', False)]}"/>
<field name="url"
attrs="{'required': [('image', '=', False)], 'invisible': [('slide_type', 'not in', ('document', 'presentation', 'video'))]}" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please coordonnate yourself with another guy that chipotated with those at 986267c
Histoire d'uniformiser et qu'on ait le meme commit. Premier qui merge a gagné, as usual.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I coordinated my chipoterie with the concerned dude sir.

@awa-odoo awa-odoo force-pushed the master-slides-survey-bridge-awa branch 2 times, most recently from 5f09598 to 7dfbd99 Compare February 14, 2019 14:38
Copy link
Contributor

@jem-odoo jem-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny review yeay


report_sudo = request.env.ref('survey.certification_report').sudo()

report = getattr(report_sudo, 'render_qweb_pdf')([succeeded_attempt.id], data={'report_type': 'pdf'})[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why a getattr here ? O_0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, failed copy/paste. Fixed it!

@@ -127,6 +127,7 @@ class Slide(models.Model):
partner_ids = fields.Many2many('res.partner', 'slide_slide_partner', 'slide_id', 'partner_id',
string='Subscribers', groups='base.group_website_publisher')
slide_partner_ids = fields.One2many('slide.slide.partner', 'slide_id', string='Subscribers information', groups='base.group_website_publisher')
slide_partner_id = fields.Many2one('slide.slide.partner', string="Subscriber information", compute='_compute_slide_partner_id')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

help message or comment in compute method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Help message is indeed nice to have.

'category': 'Hidden',
'version': '0.1',

'depends': ['website_slides', 'survey'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

website_survey instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done sir.

from odoo.http import request


class WebsiteSlides(WebsiteSlides):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we split controller into separated file ? like survery.py, slides.py ....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split and good to go!

@awa-odoo awa-odoo force-pushed the master-slides-survey-bridge-awa branch from 7dfbd99 to 1b10278 Compare February 14, 2019 15:28
@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Feb 14, 2019
robodoo added a commit that referenced this pull request Feb 15, 2019
This merge proposes a new homepage for slides module evolving towards
an eLearning platform. This merge closes task 1936153 and PR #30770.
More generally this merge is linked to currently under heavy work slides
module update to eLearning [1][2].

It includes

 * a new main page displaying top courses / channels. It displays 3 most
   popular and newest channels as well as its ongoing courses (if logged).
   Achievements and karma update done by eLearning community allow to
   give a gamification look and feel;
 * an 'All' page displaying all courses / channels. Tag groups and tags
   allow to search / filter displayed courses;
 * a new pimped and revamped display for main course / channel page;
 * new demo data for slides and small addition of demo data in bridge module
   with website_sale and website_forum;

Some features may not be completely working as other tasks will continue to
improve the display, notably new slide display [1], some forum / slide
link improvements [3] and certification integration [4]. Some code cleaning
will come in future update as this merge allows to unblock those other tasks.

For more details see sub commits. And thanks to the RD Fun SM Team. Much
fun, much SM.

[1] main eLearning task: task ID 1902304 - PR #29876
[2] new user profile /gamification: task ID 1922159 - PR #30514
[3] forum and comments upgrade: task ID 1940516 - PR #30514 and #31097
[4] certification inside eLearning: task ID 1940360 - PR #31060
Copy link
Contributor

@tde-banana-odoo tde-banana-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technical review

('id', '=', survey_id),
('certificate', '=', True)
])
survey.ensure_one()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would return to / or /web if not survey.exists() . It is clearer than using ensure_one I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done it with if not survey:

@@ -501,6 +509,34 @@ def survey_report(self, survey, answer_token=None, **post):
# filter_finish: boolean => only finished surveys or not
#

@http.route(['/survey/download_certification/<int:survey_id>'], type='http', auth='user', methods=['POST'], website=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/survey/int/get_certification ? So that we namespace a bit survey routes. I think we alread had this discussion; for new routes like this one I think we don't have to be like old routes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the route to fit the nice namespace.

@@ -127,6 +127,8 @@ class Slide(models.Model):
partner_ids = fields.Many2many('res.partner', 'slide_slide_partner', 'slide_id', 'partner_id',
string='Subscribers', groups='base.group_website_publisher')
slide_partner_ids = fields.One2many('slide.slide.partner', 'slide_id', string='Subscribers information', groups='base.group_website_publisher')
slide_partner_id = fields.Many2one('slide.slide.partner', string="Subscriber information", compute='_compute_slide_partner_id',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this name confusing. user_membership_id, current_slide_partner_id, current_subscription_id, .. ? I don't have any good idea. slide_partner_id is unclear compared to slide_partner_ids .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have any nicer proposition so I went with "user_membership_id".

@@ -153,7 +153,7 @@
<div class="row" t-if="not search">
<!-- Navigation -->
<div class="col-lg-8 col-md-12 col-12">
<ul class="nav nav-tabs nav-tabs-border" role="tablist">
<ul class="nav nav-tabs nav-tabs-border o_wslides_channel_type_nav_tabs" role="tablist">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be necessary I think with shiny '%ss' mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Praise the '%ss'

</xpath>
</template>

<template id="slides_channel_header_inherit_survey" inherit_id="website_slides.slides_channel_header">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be necessary I think with shiny '%ss' mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Praise the '%ss'

def _action_set_viewed(self, target_partner):
""" If the slide viewed is a certification, we initialize the first survey.user_input
for the current partner. """
created_slide_partners = super(Slide, self)._action_set_viewed(target_partner)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_slide_partners ? We gain 3 characters :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes <- this is also 3 characters btw

class SlidePartnerRelation(models.Model):
_inherit = 'slide.slide.partner'

user_input_ids = fields.One2many('survey.user_input', 'slide_partner_id', 'Certification attempts')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I don't really like this field as there are access rights limitations and I think it is not really useful. In order to use it you will have to be in sudo mode or have survey officer / manager group. I think we can achieve current result without the o2m (I will update / check others occurrences of it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Common you know you like it.

('survey_id', '!=', False)
])

for created_slide_partner in created_slide_partners:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could simplify a bit with something like (à pouf)

existing_answers = self.env['survey.user_input'].sudo().search([('partner_id',  '=', target_partner.id), ('survey_id', 'in', new_slide_partners.mapped('slide_id.survey_id').ids)])
new_surveys = self.mapped('survey_id') - existing_answers.mapped('survey_id')
if new_surveys:
    # create answer

If we want to batch on target partners we will have to do something smarter but this is currently not the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's not necessary anymore after our discussion?

from odoo import fields, models


class SurveyUserInput(models.Model):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a small explanation about having slide_partner_id (member) or slide_id (test entry for publishers, should not happen in other cases).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some "help" parameters on those two.

@@ -0,0 +1,13 @@
<?xml version="1.0" encoding="utf-8"?>
<odoo>
<record id="slide_channel_view_form" model="ir.ui.view">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such naming 😍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

Task #1940360
Subtask of #1902304

Purpose
=======

Some fields displayed on the slide.slide form view are not relevant
for all slide_types and should be hidden.
Purpose
=======

Website publisher should be allowed to access slides on the frontend even if
he's not a member of the channel.
Task #1940360
Subtask of #1902304

Purpose
=======

This commit adds a route allowing to download a certification for
a user that has succeeded it.

It's currently not used in any views but it's a preliminary work
for the future slide bridge.
@awa-odoo awa-odoo force-pushed the master-slides-survey-bridge-awa branch from 1b10278 to 2be23bf Compare February 15, 2019 14:08
@robodoo robodoo removed the CI 🤖 Robodoo has seen passing statuses label Feb 15, 2019
Task #1940360
Subtask of #1902304

Purpose
=======

Adds certification capabilities to the website_slides module.
Channel/courses can now include certifications as a new type of slide.

This new type of slide is available as a "Certification" button on the slide creation
frontend view (next to "Video", "Presentation", ...).
Users have to link the slide to an actual survey that has the 'certificate' field
set to true (that will populate the slide's survey_id field).

Slides of type certification are handled in frontend in a very simple way for now:
- A button "Begin certification" that redirects the user to the related survey frontend.
- A button "Download certification" when the user has succeeded the certification.
- When the survey is done, if it's linked to a slide, a button "Go back to course" allows
  the user to go back to the slide frontend.

(There is a special use case for when the website_publisher designing the survey lands on a
certification slide: he is allowed to test the survey with a survey_input created as test_entry)

Survey creation as well as limited time, limited number of attempts, ... are still completely
handled in the survey module. That means that the user will have to first create a suitable survey
that is a certification and only then create a slide of type "certification" and link
the created survey to it.

Ideally, the taking of the survey should be transparently included in the slide frontend but
it requires a full refactoring of the way surveys are submitted. This will most likely come in
a later commit.

This commit is an advanced merge of full eLearning module (see task #1902304).
Purpose
=======

Channel statistics computation in the _compute_slides_statistics method
did not work for a batch of channels.

(The stats dict was shared for all the channels).
@awa-odoo awa-odoo force-pushed the master-slides-survey-bridge-awa branch from 2550ce4 to f73cd7c Compare February 15, 2019 15:40
@tde-banana-odoo
Copy link
Contributor

@robodoo r+ rebase-merge

@robodoo
Copy link
Contributor

robodoo commented Feb 15, 2019

Merge method set to rebase and merge, using the PR as merge commit message

@tde-banana-odoo
Copy link
Contributor

Thanks @robodoo you are handsome

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Feb 15, 2019
robodoo added a commit that referenced this pull request Feb 15, 2019
…d survey

Task #1940360
Subtask of #1902304

Purpose
=======

Adds certification capabilities to the website_slides module.
Channel/courses can now include certifications as a new type of slide.

This new type of slide is available as a "Certification" button on the slide creation
frontend view (next to "Video", "Presentation", ...).
Users have to link the slide to an actual survey that has the 'certificate' field
set to true (that will populate the slide's survey_id field).

Slides of type certification are handled in frontend in a very simple way for now:
- A button "Begin certification" that redirects the user to the related survey frontend.
- A button "Download certification" when the user has succeeded the certification.
  (A new download route has been added in the survey module to make it work.)
- When the survey is done, if it's linked to a slide, a button "Go back to course" allows
  the user to go back to the slide frontend.

(There is a special use case for when the website_publisher designing the survey lands on a
certification slide: he is allowed to test the survey with a survey_input created as test_entry)

Survey creation as well as limited time, limited number of attempts, ... are still completely
handled in the survey module. That means that the user will have to first create a suitable survey
that is a certification and only then create a slide of type "certification" and link
the created survey to it.

Ideally, the taking of the survey should be transparently included in the slide frontend but
it requires a full refactoring of the way surveys are submitted. This will most likely come in
a later commit.

This commit is an advanced merge of full eLearning module (see task #1902304).

closes #31060
@robodoo
Copy link
Contributor

robodoo commented Feb 15, 2019

Merged, thanks!

@robodoo robodoo closed this Feb 15, 2019
@fw-bot fw-bot deleted the master-slides-survey-bridge-awa branch October 19, 2019 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI 🤖 Robodoo has seen passing statuses RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants