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

[IMP] website_event: move event question and add identification questions #112164

Conversation

pko-odoo
Copy link
Contributor

@pko-odoo pko-odoo commented Feb 8, 2023

This pr moves the code from website_event_questions to website_event and from website_event_crm_questions to website_event_crm module.

And it changes the way the identification questions (name, email, phone) are asked when registering to an event.
They aren't hardcoded anymore and can be created per event the same way other questions can be.
They can be set as mandatory or not and the order can be changed.
One can now also ask for the attendee company name.


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

@robodoo
Copy link
Contributor

robodoo commented Feb 8, 2023

Pull request status dashboard

@C3POdoo C3POdoo requested a review from a team February 8, 2023 08:55
@C3POdoo C3POdoo added the RD research & development, internal work label Feb 8, 2023
@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch 5 times, most recently from acd8654 to 8ba3c6b Compare February 14, 2023 15:37
@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch 2 times, most recently from e8647c1 to 7b180b2 Compare February 20, 2023 10:46
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.

First validation ! Looked at global models, added some comments :) as explained on task, first step will anyway be to split the mov from the ref/imp/code cleaning, to keep history and allow to better understand changes.

Once updated and current comments answered/done, will have another look :)

Thanks and cheers !

@@ -34,6 +34,7 @@ class EventRegistration(models.Model):
email = fields.Char(string='Email', compute='_compute_email', readonly=False, store=True, tracking=11)
phone = fields.Char(string='Phone', compute='_compute_phone', readonly=False, store=True, tracking=12)
mobile = fields.Char(string='Mobile', compute='_compute_mobile', readonly=False, store=True, tracking=13)
company = fields.Char(string='Company Name')
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be a new field (don't find it in current website_event_question). In which case it should have its own commit and explanation :)

addons/event/models/event_registration.py Outdated Show resolved Hide resolved
addons/website_event/models/event_event.py Show resolved Hide resolved
addons/website_event/models/event_event.py Show resolved Hide resolved
addons/website_event/models/event_question.py Show resolved Hide resolved
addons/website_event/models/event_question.py Outdated Show resolved Hide resolved
@@ -171,4 +171,17 @@
</field>
</record>

<function model="event.type" name="write">
Copy link
Contributor

Choose a reason for hiding this comment

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

Have to say this is louche ... we generally don't update customer data even in demo mode. When using Odoo in demo mode, you will probably directly install website_event (which is the main app), which means we should have questions on demo data if we want but I don't think we need to introspect existing data and update it. Would prefer sticking on having questions on demo event types and let customer data untouched.

addons/website_event/models/event_registration.py Outdated Show resolved Hide resolved

@api.constrains('question_ids')
def _check_unique_identification_question_ids(self):
for event in self:
Copy link
Contributor

Choose a reason for hiding this comment

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

Like the other one, maybe an sql constraint on event_type_id/question_type uniqueness would be better, if possible ?

addons/website_event/views/event_question_views.xml Outdated Show resolved Hide resolved
@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch 3 times, most recently from 93f5c75 to 70e6e98 Compare March 28, 2023 08:03
Copy link
Contributor Author

@pko-odoo pko-odoo left a comment

Choose a reason for hiding this comment

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

For the constraints, I used a sql constraints in event.question model init method but the downside of this is that I can't set a custom message to it 🥲

addons/website_event/models/event_question.py Outdated Show resolved Hide resolved
addons/website_event/views/event_question_views.xml Outdated Show resolved Hide resolved
if 'question_type' in vals:
questions_new_type = self.filtered(lambda question: question.question_type != vals['question_type'])
if questions_new_type:
answer_count = self.env['event.registration.answer'].search_count([('question_id', 'in', questions_new_type.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.

answer_ids points towards event.question.answer not event.registration.answer. Here we are looking for the already answered questions to avoid deleting them. Or am I missing something ?

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.

Second check, but some questions arise during the validation, going back to the task to ask them :) .

addons/website_event/data/event_registration_demo.xml Outdated Show resolved Hide resolved
addons/website_event/data/event_question_demo.xml Outdated Show resolved Hide resolved
addons/website_event/tests/__init__.py Outdated Show resolved Hide resolved
addons/website_event/views/event_question_views.xml Outdated Show resolved Hide resolved
addons/website_event/views/event_question_views.xml Outdated Show resolved Hide resolved
addons/event/models/event_registration.py Outdated Show resolved Hide resolved
addons/website_event/models/event_question.py Outdated Show resolved Hide resolved
@api.depends('question_type')
def _compute_title(self):
for question in self:
question_type_dict = dict(self._fields['question_type'].selection)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would that be translated ?

addons/website_event/views/event_type_views.xml Outdated Show resolved Hide resolved
@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch 2 times, most recently from 0c1cd15 to 137a26b Compare April 20, 2023 15:00
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.

Zboing, I think it is ok for the validation, especially if we want it for the freeze ^^ few comments, but not blocking for testing :)

@@ -34,6 +34,7 @@ class EventRegistration(models.Model):
email = fields.Char(string='Email', compute='_compute_email', readonly=False, store=True, tracking=11)
phone = fields.Char(string='Phone', compute='_compute_phone', readonly=False, store=True, tracking=12)
mobile = fields.Char(string='Mobile', compute='_compute_mobile', readonly=False, store=True, tracking=13)
company_name = fields.Char(string='Company Name', compute='_compute_company_name', readonly=False, store=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't remember if already asked, but no tracking on this field ?

@@ -36,6 +60,10 @@
</div>
</div>
</field>
<field name="question_ids" eval="[
(0, 0, {'title': 'Name', 'question_type': 'name', 'is_mandatory_answer': True}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would do a [(5, 0), (0, 0, {...}, ..., otherwise I fear that at each -u / -i on an existing db questions pile up (or a constraint is raised because they already exists).


def init(self):
# Allows only 1 same identification question per event / event type
self.env.cr.execute("""
Copy link
Contributor

Choose a reason for hiding this comment

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

Shoudln't we also have a constraint about "either event_id, either event_type_id" ?

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 you mean like this

@api.constrains('event_type_id', 'event_id')
? ^^

Copy link
Contributor

@awa-odoo awa-odoo left a comment

Choose a reason for hiding this comment

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

Hello @pko-odoo

Not fully tested yet but here is a preliminary review so we can try to squeeze this for the freeze.

I'm already submitting those comments as they are about the MOV part.
Will make a second batch with the comments on the other commit so it's easier for you.

Thanks & cheers!

if counter == '0':
global_values[attr_name] = value
if 'question_answer' in key and value:
dummy, registration_index, question_id = key.split('-')
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: you moved the code but not the associated docstring of this method, could be nice to keep it during the MOV commit.

@@ -7,3 +7,9 @@ access_event_tag,event.tag,event.model_event_tag,,1,0,0,0
access_website_event_menu,website.event.menu,model_website_event_menu,,1,0,0,0
access_website_event_menu_user,website.event.menu.user,model_website_event_menu,event.group_event_user,1,1,1,1
access_website_visitor_user,website.visitor.user,model_website_visitor,event.group_event_user,1,1,0,0
access_website_event_question,event.question,model_event_question,,1,0,0,0
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why did you add the "website_" prefix?

Can't we keep the same names as before?

@@ -29,6 +31,87 @@ def test_website_event_pages_seo(self):
self.assertEqual(intro_event_menu.view_id.website_meta_title, "Hello, world!")
self.assertEqual(event.website_meta_title, False)

def test_01_tickets_questions(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename to "test_website_event_questions" to match naming of other tests?

</record>

<record id="event_registration_view_search" model="ir.ui.view">
<field name="name">event.registration.view.search.inherit.website.event</field>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like above naming chose "inherit.online".

@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch from 137a26b to b63d087 Compare May 8, 2023 09:05
Copy link
Contributor

@awa-odoo awa-odoo left a comment

Choose a reason for hiding this comment

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

Hello @pko-odoo

Finished a first reading of the second commit.

Not a fan of the title magic with the "identification questions", would simplify that part.
See review comment for details and suggestions.

Rest is pretty straightforward, added some general comments about naming / test coverage / ...

Thanks for your work & cheers!

@@ -34,6 +34,7 @@ class EventRegistration(models.Model):
email = fields.Char(string='Email', compute='_compute_email', readonly=False, store=True, tracking=11)
phone = fields.Char(string='Phone', compute='_compute_phone', readonly=False, store=True, tracking=12)
mobile = fields.Char(string='Mobile', compute='_compute_mobile', readonly=False, store=True, tracking=13)
company_name = fields.Char(string='Company Name', compute='_compute_company_name', readonly=False, store=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a small note in the commit message? Seems like we add information on the attendee as well (and not only refactor the questions stuff).

addons/website_event/controllers/main.py Outdated Show resolved Hide resolved
<t t-if="availability_check">
<t t-set="counter" t-value="0"/>
<t t-set="input_type" t-value="{'name': 'text', 'email': 'email', 'phone': 'tel', 'company_name': 'text'}"/>
<t t-set="type_labels" t-value="dict(env['event.question']._fields['question_type'].selection)"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure those are "untranslated".

Wouldn't it be easier to keep the title field for "identification questions" in the form view and just initialize it if empty to what we want?

It would simplify and also allow end users to just change the questions label if they want to (so "Mobile" instead of "Phone" for example).

Probably to confirm with Luc.

def init(self):
# Allows only 1 same identification question per event / event type
self.env.cr.execute("""
CREATE UNIQUE INDEX IF NOT EXISTS event_unique_identification_question
Copy link
Contributor

Choose a reason for hiding this comment

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

Have to say that creates very ugly error messages on the form view :D

Can't we go for a custom api.constrains to control a bit the error messages?

if question.question_type not in ['simple_choice', 'text_box']:
question.title = question_type_dict.get(question.question_type)
elif question.title in question_type_dict.values():
question.title = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Have to say I also don't understand the purpose, this seems rather complicated just for a title.

Would say let people enter what they want and initialize maybe for the "identification questions" if we want to. See other comment.

(Which makes me wonder if we even need a "is_identification_question" field).

addons/website_event/models/event_type.py Outdated Show resolved Hide resolved
['simple_choice', 'simple_choice', 'text_box'])
self.assertEqual(event.specific_question_ids.title, 'Question1')
['name', 'email', 'phone', 'simple_choice', 'simple_choice', 'text_box'])
self.assertEqual(event.specific_question_ids.filtered(
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also adapt to test the title of the added questions (name / phone / email) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is a few lines below ^^

@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch from b63d087 to df6a917 Compare May 9, 2023 09:33
@@ -350,6 +334,10 @@
<template id="registration_event_question" name="Registration Event Question">
<label t-out="question.title"/>
<span t-if="question.is_mandatory_answer">*</span>
<t t-if="question.is_identification_question">
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like "is_identification_question" is only used in tests and here in the end.

Can't we instead use input_type_by_question_type.get(question.question_type, 'some_default_value') ?

And in tests adapt a bit so that we don't need this field? Feels a bit overkill.

@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch from df6a917 to b9cf68b Compare May 10, 2023 09:11
@pko-odoo pko-odoo changed the title [IMP] website_event: migrate website_event_questions to this module [IMP] website_event: move event question and add identification questions May 10, 2023
@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch 3 times, most recently from 30d1922 to 9221436 Compare May 10, 2023 14:56
Comment on lines 273 to 267
global_values = {}
general_answer_ids = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @pko-odoo

Looking at current master code I think I found the answer to our earlier question.

Actually it seems that website_event had this logic:

image

And website_event_questions had this logic:

image

Which means that "global_values" was holding the field values (name / email / phone) and "general_answer_ids" was holding the question (selection and text) values.

In your current version, as you don't have specific questions for field values, you don't need "global values" anymore.

Just wanted to type this before I forget when we re-discuss tomorrow 😄
In short, it's normal, we're good.

Cheers!

Copy link
Contributor

@awa-odoo awa-odoo left a comment

Choose a reason for hiding this comment

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

Hello @pko-odoo

Last bits of polishing and then we should be good to go 🚀

Thanks for keeping up with me on this & cheers!

addons/website_event/controllers/main.py Outdated Show resolved Hide resolved
addons/website_event/controllers/main.py Show resolved Hide resolved
addons/website_event/controllers/main.py Outdated Show resolved Hide resolved
addons/website_event/controllers/main.py Show resolved Hide resolved
addons/website_event/controllers/main.py Outdated Show resolved Hide resolved
addons/website_event/controllers/main.py Outdated Show resolved Hide resolved
['simple_choice', 'simple_choice', 'text_box'])
self.assertEqual(event.specific_question_ids.title, 'Question1')
['name', 'email', 'phone', 'simple_choice', 'simple_choice', 'text_box'])
self.assertEqual(event.specific_question_ids.filtered(
Copy link
Contributor

Choose a reason for hiding this comment

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

Up?

'question_answer-2-%s' % self.event_question_1.id: '9',
'question_answer-0-%s' % self.event_question_2.id: '7',
'question_answer-0-%s' % self.event_question_3.id: 'Free Text',
'1-name-%s' % self.event_identification_question_1.id: 'Pixis',
Copy link
Contributor

Choose a reason for hiding this comment

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

As we have a heuristic to test that we pick the first value for multiple identification questions, could be interesting to have 2 phone questions for example, and make sure we populate the field with the first answer.

Would also be nice to include the company_name into this.

@awa-odoo
Copy link
Contributor

( @pko-odoo Looks like runbot is still exploding by the way so make sure to re-run all tests locally and fix them 🙂 )

@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch 5 times, most recently from 08d67c6 to 8f71c43 Compare May 11, 2023 11:48
@awa-odoo
Copy link
Contributor

@robodoo r+ rebase-ff

@robodoo
Copy link
Contributor

robodoo commented May 11, 2023

Merge method set to rebase and fast-forward.

@robodoo
Copy link
Contributor

robodoo commented May 11, 2023

@pko-odoo @awa-odoo linked pull request(s) odoo/upgrade#4313 not ready. Linked PRs are not staged until all of them are ready.

@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch 2 times, most recently from e2dda20 to 77fbbc8 Compare May 11, 2023 20:59
@awa-odoo
Copy link
Contributor

@robodoo r+

@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch from 77fbbc8 to 7230400 Compare May 12, 2023 07:44
@awa-odoo
Copy link
Contributor

@robodoo r+

pko-odoo added 2 commits May 12, 2023 12:45
This commit moves the code from website_event_questions to
website_event and from website_event_crm_questions to
website_event_crm module.

Task-3056380
This commit changes the way the identification questions (name,
email, phone) are asked when registering to an event. They aren't
hardcoded anymore and can be created per event the same way other
questions can be. They can be set as mandatory or not and the order
can be changed. One can now also ask for the attendee company name.

Task-3056380
@pko-odoo pko-odoo force-pushed the master-website-event-migrate-questions-pko branch from 7230400 to 91417e1 Compare May 12, 2023 10:46
@awa-odoo
Copy link
Contributor

@robodoo r+

robodoo pushed a commit that referenced this pull request May 12, 2023
This commit moves the code from website_event_questions to
website_event and from website_event_crm_questions to
website_event_crm module.

Task-3056380

Part-of: #112164
robodoo pushed a commit that referenced this pull request May 12, 2023
This commit changes the way the identification questions (name,
email, phone) are asked when registering to an event. They aren't
hardcoded anymore and can be created per event the same way other
questions can be. They can be set as mandatory or not and the order
can be changed. One can now also ask for the attendee company name.

Task-3056380

closes #112164

Related: odoo/upgrade#4313
Signed-off-by: Warnon Aurélien (awa) <awa@odoo.com>
@robodoo robodoo temporarily deployed to merge May 12, 2023 20:52 Inactive
@robodoo robodoo closed this May 12, 2023
@robodoo robodoo added the 16.3 label May 12, 2023
@fw-bot fw-bot deleted the master-website-event-migrate-questions-pko branch May 26, 2023 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
16.3 RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants