[10.0][NEW] event_registration_multi_qty: New module for allow registration writing quantities. #78

sergio-teruel · 2017-06-02T08:56:10Z

yajo

I have a lot of suggested improvements, but none of them seem to be blocking. Code review OK.

PS: I guess this will have to be combined with a website_event_sale_registration_qty addon, right?

yajo · 2017-06-02T09:51:36Z

event_registration_multi_qty/README.rst

+    :alt: License: AGPL-3
+
+============================
+Event Registration Multi Qty


IMHO event_registration_qty should be enough. The same for this title.

yajo · 2017-06-02T09:52:04Z

event_registration_multi_qty/README.rst

@@ -0,0 +1,56 @@
+.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
+    :alt: License: AGPL-3


Could you update to latest template please?

yajo · 2017-06-02T09:52:29Z

event_registration_multi_qty/README.rst

+Event Registration Multi Qty
+============================
+
+This module allows to make registration to events with more than one quantity.


"more than one quantity" -> "more than one attendee".

yajo · 2017-06-02T09:52:54Z

event_registration_multi_qty/__manifest__.py

+    'author': 'Tecnativa, '
+              'Odoo Community Association (OCA)',
+    "license": "AGPL-3",
+    'website': 'https://odoo-community.org/',


https://www.tecnativa.com

yajo · 2017-06-02T09:57:37Z

event_registration_multi_qty/models/event.py

+    _inherit = 'event.event'
+
+    registration_multi_qty = fields.Boolean(
+        string='Registration Multi Qty',


"Allow multiple attendees per registration"

yajo · 2017-06-02T09:58:00Z

event_registration_multi_qty/models/event.py

+
+    registration_multi_qty = fields.Boolean(
+        string='Registration Multi Qty',
+        help='Allow more than one registration'


With the above string you do not need this help I think

yajo · 2017-06-02T10:00:31Z

event_registration_multi_qty/models/event.py

+    @api.onchange('registration_multi_qty')
+    def _onchange_registration_multi_qty(self):
+        if not self.registration_multi_qty:
+            self.registration_ids.write({'qty': 1})


Calling write under an onchange is dangerous, because onchanges work in a draft record while write don't. You should call update instead.

Anyway, this does not seem to be a good choice. If somebody clicks here by mistake, he'd be overriding all qtys. I feel for this is better to add a constraint, so that if you already have registrations with qty != 1 you cannot check this box. The same constraint could be called when changing qty field in registrations.

Further reading showed me that after all, qty is ignored if disabled, so why even bother about this?

yajo · 2017-06-02T10:15:31Z

event_registration_multi_qty/models/event.py

+            if not event.registration_multi_qty:
+                return super(EventEvent, self)._compute_seats()
+            event.seats_unconfirmed = event.seats_reserved = 0
+            event.seats_used = event.seats_available = 0


In these 2 lines you are actually performing 4 writes to DB. Later below, each += call performs another write. How about working with simple variables that start with 0 and perform a single .write({...}) at the end? 🤔

yajo · 2017-06-02T10:18:49Z

event_registration_multi_qty/models/event.py

+        default=1,
+    )
+    registration_multi_qty = fields.Boolean(
+        related='event_id.registration_multi_qty')


I'd add readonly=True here too.

sergio-teruel · 2017-06-02T11:59:08Z

@yajo Changes done, no changes in module name

yajo · 2017-06-02T14:11:08Z

event_registration_multi_qty/models/event.py

+                event.seats_unconfirmed + event.seats_reserved +
+                event.seats_used)
+
+    @api.multi


AFAIK this is no more needed in v10. Not a requirement though.

yajo · 2017-06-02T14:11:59Z

event_registration_multi_qty/models/event.py

+            if not event.registration_multi_qty and \
+                    max(event.registration_ids.mapped('qty') or [0]) > 1:
+                raise ValidationError(
+                    _('You can not inactive this option if there is '


inactive > disable
is > are

yajo · 2017-06-02T14:12:37Z

event_registration_multi_qty/models/event.py

+                    max(registration.event_id.registration_ids.mapped(
+                        'qty') or [0]) > 1:
+                raise ValidationError(
+                    _('You can not add quantities if you not active the'


active > activate

yajo · 2017-06-02T14:18:35Z

event_registration_multi_qty/models/event.py

+        for registration in self:
+            if not registration.event_id.registration_multi_qty and \
+                    max(registration.event_id.registration_ids.mapped(
+                        'qty') or [0]) > 1:


This check is IMHO overloaded. While editing a registration, you don't need to check all others from the same event, since they don't changed.

if True in self.mapped("event_id.registration_multi_qty") and self.filtered(lambda reg: reg.qty != 1) should be enough

yajo · 2017-06-02T14:20:03Z

Check Travis too please

sergio-teruel · 2017-06-02T14:34:41Z

@yajo Changes done

chienandalu · 2017-06-06T13:19:28Z

event_registration_multi_qty/views/event_view.xml

+    <field name="inherit_id" ref="event.view_event_form"/>
+    <field name="arch" type="xml">
+        <field name="auto_confirm" position="after">
+            <field name="registration_multi_qty"/>


Not a good hook since event_sale overrides it. Any better position?

sergio-teruel · 2017-06-07T06:55:37Z

@chienandalu changes done

pedrobaeza · 2017-06-09T09:57:21Z

event_registration_multi_qty/README.rst

+
+To use this module, you need to:
+
+#. Go to *Events* and create one.


These are configuration steps, not usage steps.

I think these are usage steps: you need to create an event to use this addon.

pedrobaeza · 2017-06-09T09:57:43Z

event_registration_multi_qty/README.rst

+#. Go to *Events* and create one.
+#. Go to *Registration tab* and check "Registration Multi Qty" to activate
+   this function.
+#. Create an attendee for this event and you can write the quantities for this


This one should be kept here though.

pedrobaeza · 2017-06-09T10:02:40Z

event_registration_multi_qty/models/event.py

+    @api.depends('seats_max', 'registration_ids.state',
+                 'registration_ids.qty')
+    def _compute_seats(self):
+        for event in self:


Better for performance reasons on super:

multi_qty_events = self.filtered('registration_multi_qty') for event in multi_qty_events: ... rest = self - multi_qty_events super(EventEvent, rest)._compute_seats()

pedrobaeza · 2017-06-09T10:05:54Z

event_registration_multi_qty/models/event.py

+    def _compute_seats(self):
+        for event in self:
+            if not event.registration_multi_qty:
+                return super(EventEvent, self)._compute_seats()


Take care with this next time! (although now with the new code proposed this dissapears). This would end the computation in the middle if you find an event without multi qty.

pedrobaeza · 2017-06-09T10:08:03Z

event_registration_multi_qty/models/event.py

+                    max(event.registration_ids.mapped('qty') or [0]) > 1:
+                raise ValidationError(
+                    _('You can not disable this option if there are '
+                      'registrations with quantities grather than one.'))


s/grather/greater (and change source translation also).

pedrobaeza · 2017-06-09T10:08:50Z

event_registration_multi_qty/models/event.py

+    @api.multi
+    @api.constrains('registration_multi_qty')
+    def _check_attendees_qty(self):
+        for event in self:


for event in self.filtered(lambda x: not x.registration_multi_qty): and remove the condition inside the loop.

pedrobaeza · 2017-06-09T10:12:25Z

event_registration_multi_qty/models/event.py

+    @api.multi
+    @api.constrains('qty')
+    def _check_attendees_qty(self):
+        for registration in self:


Put directly:
if self.filtered(lambda x: not x.event_id.registration_multi_qty and registration.qty > 1):

Oh well, thiking it twice... 🤔 @sergio-teruel is being more performant here, because the loop stops on the first match, so he better leaves this as it is. 👍

pedrobaeza · 2017-06-09T11:56:03Z

event_registration_multi_qty/reports/report_event_registration.py

+    """Events Analysis"""
+    _inherit = "report.event.registration"
+
+    def _sub_select(self):


This won't work if odoo/odoo#16633 is not merged, so we have to wait.

yajo · 2017-06-12T07:27:27Z

event_registration_multi_qty/README.rst

+
+To use this module, you need to:
+
+#. Go to *Events* and create one.


I think these are usage steps: you need to create an event to use this addon.

yajo · 2017-06-12T07:27:56Z

event_registration_multi_qty/README.rst

+
+.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
+   :alt: Try me on Runbot
+   :target: https://runbot.odoo-community.org/runbot/199/10


Broken link.

…ties at once

pedrobaeza · 2017-06-21T10:22:26Z

@yajo, please give your final blessing.

yajo

Awesome 👍

chienandalu

Reports don't count properly on runbot (new report query not yet propagated?). Tested locally with success 😃

sergio-teruel force-pushed the 10.0-PR-event_registration_multi_qty branch from d910e61 to c03af50 Compare June 2, 2017 09:22

yajo approved these changes Jun 2, 2017

View reviewed changes

sergio-teruel force-pushed the 10.0-PR-event_registration_multi_qty branch from c03af50 to 4354e48 Compare June 2, 2017 11:58

yajo requested changes Jun 2, 2017

View reviewed changes

sergio-teruel force-pushed the 10.0-PR-event_registration_multi_qty branch from 4354e48 to cea2926 Compare June 2, 2017 14:34

sergio-teruel force-pushed the 10.0-PR-event_registration_multi_qty branch 2 times, most recently from 878199e to ab16f62 Compare June 2, 2017 14:43

yajo approved these changes Jun 5, 2017

View reviewed changes

chienandalu reviewed Jun 6, 2017

View reviewed changes

OCA deleted a comment from yajo Jun 9, 2017

pedrobaeza requested changes Jun 9, 2017

View reviewed changes

yajo requested changes Jun 12, 2017

View reviewed changes

pedrobaeza force-pushed the 10.0-PR-event_registration_multi_qty branch from 340a7b3 to c455f10 Compare June 20, 2017 18:11

sergio-teruel and others added 2 commits June 20, 2017 20:25

[ADD] event_registration_multi_qty: Allow registering multiple quanti…

368d7b5

…ties at once

[FIX] event_registration_multi_qty: Needed changes

4c8bd0c

pedrobaeza force-pushed the 10.0-PR-event_registration_multi_qty branch from c455f10 to 4c8bd0c Compare June 20, 2017 18:27

OCA deleted a comment from yajo Jun 20, 2017

pedrobaeza approved these changes Jun 20, 2017

View reviewed changes

yajo approved these changes Jun 22, 2017

View reviewed changes

chienandalu approved these changes Jun 22, 2017

View reviewed changes

chienandalu mentioned this pull request Jun 22, 2017

[10.0][ADD] event_sale_registration_multy_qty: New module #81

Merged

1 task

yajo merged commit 9edaee8 into OCA:10.0 Jun 22, 2017

yajo deleted the 10.0-PR-event_registration_multi_qty branch June 22, 2017 10:16

		@@ -0,0 +1,56 @@
		.. image:: https://img.shields.io/badge/licence-AGPL--3-blue.svg
		:alt: License: AGPL-3


		To use this module, you need to:

		#. Go to Events and create one.

[10.0][NEW] event_registration_multi_qty: New module for allow registration writing quantities. #78

[10.0][NEW] event_registration_multi_qty: New module for allow registration writing quantities. #78

Conversation

sergio-teruel commented Jun 2, 2017 • edited by yajo

yajo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergio-teruel commented Jun 2, 2017

yajo Jun 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajo commented Jun 2, 2017

sergio-teruel commented Jun 2, 2017

Choose a reason for hiding this comment

sergio-teruel commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Jun 21, 2017

yajo left a comment

Choose a reason for hiding this comment

chienandalu left a comment

Choose a reason for hiding this comment

sergio-teruel commented Jun 2, 2017 •

edited by yajo

yajo Jun 2, 2017 •

edited