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

[FIX] website_slides_survey: Directly go from fullscreen to survey #31821

Closed
wants to merge 2 commits into
base: saas-12.2
from

Conversation

Projects
None yet
6 participants
@qmo-odoo
Copy link

qmo-odoo commented Mar 13, 2019

Before this commit, in fullscreen mode, clicking on the "pass certification"
button would redirect the user to the "non-fullscreen" view.

After this commit, the button directly redirects the user to the survey page

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo robodoo added the seen 🙂 label Mar 13, 2019

@C3POdoo C3POdoo added the RD label Mar 13, 2019

@robodoo robodoo added the CI 🤖 label Mar 13, 2019

@jem-odoo
Copy link
Contributor

jem-odoo left a comment

Review: architecture remarks mostly.
Good start !

Show resolved Hide resolved addons/website_slides_survey/controllers/main.py Outdated
addons/website_slides_survey/controllers/main.py Outdated

@http.route(['/slides_survey/certification_url/get'], type='json', auth='user', website=True)
def get_certification_url(self, slide_id):
slide = request.env['slide.slide'].browse(slide_id)

This comment has been minimized.

@jem-odoo

jem-odoo Mar 13, 2019

Contributor

use the fetch_slide method. It will check the access rights.
Du coup, maybe this method should be in slide.py

Show resolved Hide resolved addons/website_slides_survey/controllers/main.py Outdated
Show resolved Hide resolved addons/website_slides_survey/controllers/slides.py Outdated
Show resolved Hide resolved addons/website_slides_survey/models/slide_slide.py Outdated
Show resolved Hide resolved addons/website_slides_survey/controllers/slides.py Outdated
Show resolved Hide resolved addons/website_slides_survey/controllers/main.py Outdated
Show resolved Hide resolved ...s/website_slides_survey/static/src/js/slides_course_fullscreen_player.js Outdated
Show resolved Hide resolved ...s/website_slides_survey/static/src/js/slides_course_fullscreen_player.js Outdated
Show resolved Hide resolved ...bsite_slides_survey/views/website_slides_templates_lesson_fullscreen.xml
@awa-odoo
Copy link
Contributor

awa-odoo left a comment

Some additional remarks and questions as I don't quite understand the whole purpose/implementation.

Show resolved Hide resolved addons/website_slides/controllers/main.py Outdated
Show resolved Hide resolved addons/website_slides_survey/controllers/slides.py Outdated
addons/website_slides_survey/static/src/js/slides_course_fullscreen_player.js Outdated
}
}).then(function (data){
if (!data.error){
slide.certificationUrl = data[0].certification_url;

This comment has been minimized.

@awa-odoo

awa-odoo Mar 14, 2019

Contributor

What's the point of receiving a list if you only use the first element?

Show resolved Hide resolved ...bsite_slides_survey/views/website_slides_templates_lesson_fullscreen.xml Outdated

@qmo-odoo qmo-odoo force-pushed the odoo-dev:saas-12.2-elearning-certification-fullscreen-qmo branch Mar 14, 2019

@robodoo robodoo removed the CI 🤖 label Mar 14, 2019

Show resolved Hide resolved addons/website_slides/controllers/main.py Outdated
Show resolved Hide resolved addons/website_slides_survey/controllers/main.py Outdated
Show resolved Hide resolved addons/website_slides_survey/controllers/slides.py Outdated

@qmo-odoo qmo-odoo force-pushed the odoo-dev:saas-12.2-elearning-certification-fullscreen-qmo branch 2 times, most recently Mar 14, 2019

@robodoo robodoo added the CI 🤖 label Mar 14, 2019

@awa-odoo
Copy link
Contributor

awa-odoo left a comment

I know you're not done yet but here are some more comments.
We're getting there I think. It already looks a lot better.

Show resolved Hide resolved addons/website_slides_survey/controllers/slides.py Outdated
Show resolved Hide resolved addons/website_slides_survey/models/slide_slide.py Outdated
Show resolved Hide resolved addons/website_slides_survey/models/slide_slide.py Outdated
Show resolved Hide resolved addons/website_slides_survey/models/slide_slide.py Outdated
Show resolved Hide resolved addons/website_slides_survey/models/slide_slide.py Outdated
Show resolved Hide resolved ...s/website_slides_survey/static/src/js/slides_course_fullscreen_player.js Outdated
Show resolved Hide resolved addons/website_slides_survey/controllers/slides.py Outdated
Show resolved Hide resolved ...s/website_slides_survey/static/src/js/slides_course_fullscreen_player.js Outdated
Show resolved Hide resolved addons/website_slides_survey/static/src/xml/website_slides_fullscreen.xml Outdated
Show resolved Hide resolved addons/website_slides_survey/static/src/xml/website_slides_fullscreen.xml Outdated

@qmo-odoo qmo-odoo force-pushed the odoo-dev:saas-12.2-elearning-certification-fullscreen-qmo branch to 4928dbe Mar 14, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 14, 2019

@robodoo robodoo added the CI 🤖 label Mar 14, 2019

@qmo-odoo qmo-odoo force-pushed the odoo-dev:saas-12.2-elearning-certification-fullscreen-qmo branch from 80aff30 Mar 15, 2019

@robodoo robodoo removed the CI 🤖 label Mar 15, 2019

@qmo-odoo qmo-odoo force-pushed the odoo-dev:saas-12.2-elearning-certification-fullscreen-qmo branch Mar 15, 2019

@robodoo robodoo added the CI 🤖 label Mar 15, 2019

@qmo-odoo qmo-odoo force-pushed the odoo-dev:saas-12.2-elearning-certification-fullscreen-qmo branch Mar 15, 2019

@robodoo robodoo removed the CI 🤖 label Mar 15, 2019

@qmo-odoo qmo-odoo force-pushed the odoo-dev:saas-12.2-elearning-certification-fullscreen-qmo branch Mar 15, 2019

@robodoo robodoo added the CI 🤖 label Mar 15, 2019

@jem-odoo
Copy link
Contributor

jem-odoo left a comment

Some middle changes.
We see the end of the tunnel !

addons/website_slides/controllers/main.py Outdated
@@ -68,7 +68,7 @@ def _set_completed_slide(self, slide, quiz_attempts_inc=False):
slide.action_set_completed(quiz_attempts_inc=quiz_attempts_inc)
return True

def _get_slide_detail(self, slide):
def _get_slide_detail(self, slide, is_fullscreen=False):

This comment has been minimized.

@jem-odoo

jem-odoo Mar 15, 2019

Contributor

This argument can be removed now

addons/website_slides/controllers/main.py Outdated
@@ -487,7 +487,7 @@ def slide_view(self, slide, **kwargs):
# sidebar: update with user channel progress
values['channel_progress'] = self._get_channel_progress(slide.channel_id, include_quiz=True)

if 'fullscreen' in kwargs:
if kwargs.get('fullscreen', 0) == '1':

This comment has been minimized.

@jem-odoo

jem-odoo Mar 15, 2019

Contributor

kwargs.get('fullscreen') is enough. As the default value is False.

addons/website_slides_survey/controllers/slides.py Outdated

class WebsiteSlides(WebsiteSlides):

@http.route(['/slides_survey/slide/get_certification_url'], type='http', auth='user', website=True)
def slide_get_certification_url(self, slide_id, **kw):
fetch_res = super(WebsiteSlides, self)._fetch_slide(slide_id)

This comment has been minimized.

@jem-odoo

jem-odoo Mar 15, 2019

Contributor

self._fetch_slide() as we inherit the class

addons/website_slides_survey/controllers/slides.py Outdated
slide = fetch_res['slide']
if slide.channel_id.is_member:
slide.action_set_viewed()
url_list = slide._generate_certification_url()

This comment has been minimized.

@jem-odoo

jem-odoo Mar 15, 2019

Contributor

Do the '[slide.id]` directly, to only handle what you need

raise werkzeug.exceptions.NotFound()
slide = fetch_res['slide']
if slide.channel_id.is_member:
slide.action_set_viewed()

This comment has been minimized.

@jem-odoo

jem-odoo Mar 15, 2019

Contributor

We should remove the creation of the user_input from 'action_viewed', and do it in _generate_certification_url

addons/website_slides_survey/views/website_slides_templates_lesson.xml Outdated
<t t-if="not certification_test_entry" t-set="begin_certification_label" t-value="'Begin certification'" />
<t t-if="certification_test_entry" t-set="begin_certification_label" t-value="'Test certification'" />
<div t-if="slide.slide_type == 'certification' and not certification_done" class="col mt32 mb8 d-flex justify-content-center">
<t t-if="slide.channel_id.is_member" t-set="begin_certification_label" t-value="'Begin certification'" />

This comment has been minimized.

@jem-odoo

jem-odoo Mar 15, 2019

Contributor

Not your fault, but while we are at it: the t-value will never be translated.
Below, use a t-if/t-else and we don't need the t-att-title, not t-att-aria-label.

qmo-odoo added some commits Mar 13, 2019

[FIX] website_slides_survey: Directly go from fullscreen to survey
Before this commit, in fullscreen mode, clicking on the "pass certification"
button would redirect the user to the "non-fullscreen" view.

After this commit, the button directly redirects the user to the survey page
[FIX] website_slides(_survey): Go to survey if only one slide certifi…
…cation

Purpose of this commit:
  In case there is only one slide in the course and that unique slide
  is of type certification, clicking on the link redirects the user
  to the survey page instead of the fullscreen view

@qmo-odoo qmo-odoo force-pushed the odoo-dev:saas-12.2-elearning-certification-fullscreen-qmo branch to 57469da Mar 15, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 15, 2019

@jem-odoo

This comment has been minimized.

Copy link
Contributor

jem-odoo commented Mar 18, 2019

Landed in saas12.2 with
e8e8d7a
add45ee

@jem-odoo jem-odoo closed this Mar 18, 2019

@jem-odoo jem-odoo deleted the odoo-dev:saas-12.2-elearning-certification-fullscreen-qmo branch Mar 18, 2019

@robodoo robodoo added closed 💔 and removed CI 🤖 labels Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.