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

[11.0] [MIG] event_session #127

Merged
merged 14 commits into from
Mar 20, 2019
Merged

Conversation

ntsirintanis
Copy link
Contributor

Migrated here is only the logic of 10.0 event.session module that defines sessions for events. Aspects like separate session registration, seat availability per session, mail notifications per session that were included in event.session 10.0 are now omitted, because they are not really essential for defining a session, i.e., these are extra functionalities beyond the scope of this module.
More logic (or reintroduction of omitted aspects) may be implemented separately via a different module that depends on event.session 11.0. @NL66278 @lfreeke

chienandalu and others added 9 commits November 21, 2018 16:48
* Rename 'date' to 'date_begin' for being homogeneous with event fields
* Split tests
* Remove 'start_time' and 'end_time' as no needed
* Convert hours to UTC for avoiding timezone issues
- Fix tz in calculated name
- Fix tests
- Add event sessions count to reports
- Fixes computing update of stored value
- Fixes views and reports
- Fixes generator not creating last day sessions
- Totalizations in session tree view
- Security config
- Adds es_ES translations
- Fixes seats limits on sessions
- Fix report query
- Adds progress bar to sessions tree view
- Improves tests
- Fixes seats limit check
Currently translated at 100.0% (103 of 103 strings)

Translation: event-10.0/event-10.0-event_session
Translate-URL: https://translation.odoo-community.org/projects/event-10-0/event-10-0-event_session/nl/
@pedrobaeza pedrobaeza added this to the 11.0 milestone Nov 28, 2018
@pedrobaeza pedrobaeza mentioned this pull request Nov 28, 2018
15 tasks
@ntsirintanis ntsirintanis force-pushed the 11.0-mig-event_session branch 2 times, most recently from f0db268 to 5585ec1 Compare November 28, 2018 14:59
string='Total event sessions',
store=True,
)
seats_expected = fields.Integer(store=True)
Copy link
Member

Choose a reason for hiding this comment

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

Is the store=True needed here? Since this is not a computed field.

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

Choose a reason for hiding this comment

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

@@ -0,0 +1,3 @@

Copy link

Choose a reason for hiding this comment

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

license line

@@ -0,0 +1,46 @@
# Copyright 2017 David Vidal<david.vidal@tecnativa.com>
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
Copy link

Choose a reason for hiding this comment

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

And license lines should be https now also.

string='Total event sessions',
store=True,
)
seats_expected = fields.Integer(store=True)
Copy link

Choose a reason for hiding this comment

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

No need any more for seats_expected and seats_available expected (nor for the corresponding functions).

Copy link
Member

Choose a reason for hiding this comment

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

@ntsirintanis What's the reason behind introducing this in the event object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chienandalu Ah, that's remnant from some experimentation I did with the module. Thanks for spotting this, I have pushed a new commit that removes this overwrite.

comodel_name='event.event',
string='Event',
)
seats_min = fields.Integer(
Copy link

Choose a reason for hiding this comment

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

Do away with the seats stuff

@pedrobaeza
Copy link
Member

Sorry, but you can't rip off all that features from the module. They are needed the same as you have the event module as only one module, not event and event_registration, so 👎 to this migration while it doesn't preserve all the features we added in v10.

@NL66278
Copy link

NL66278 commented Nov 28, 2018

@pedrobaeza For us, and probably others, the idea of sessions is that you have one event where you register for (for instance a python course), but that is spread out over sessions (let us say for seven weeks). You do not register for each separate lesson.

But I do understand what you are saying. The usecase of the original authors of the module would be probably be to use an event as a kind of template, to generate several sessions with separate registration etc. Although I do not quite see why you would not generate events then and not sessions.

Would it be a solution to do the migration by renaming the module migrated/changed by @ntsirintanis into event_session_base, and then have event_session extend this to add the concepts of separate registration/availability etc.?

@pedrobaeza
Copy link
Member

@NL66278 we are the original authors of the module, so I have very clear what is the goal of the module ;) It's not that, it's for having the concept of session, which is another pass of the same event. For example, of theater shows, cinema, and so on, so registration is perfectly fit for doing that at session level.

What you are saying is that you pretend to use this as tracks, not sessions, so this is not intended for that.

And we designed this to have the opportunity to register to an event or to a session, so these features don't block you usage, and so you shouldn't split out this.

@NL66278
Copy link

NL66278 commented Nov 29, 2018

@pedrobaeza I agree then that we will have to provide the full migration. For now consider it as a work in progress. The underlying code for registrations changed a lot, so this will take more time. FYI we looked at tracks but they did not fit our usecase. Tracks are more things happening in parallel, like all the different tracks at the Odoo days.

@pedrobaeza
Copy link
Member

OK, thanks for reconsidering it. I have used in the past tracks for both things: parallel and linear "sessions", but of course it depends on the rest of requirements you have for that sessions. If you share them, maybe we can do a brain-storming about this.

@ntsirintanis ntsirintanis force-pushed the 11.0-mig-event_session branch 5 times, most recently from 47171fe to b590c55 Compare December 6, 2018 14:29
@ntsirintanis
Copy link
Contributor Author

I have gradually reintroduced all features that were removed for the initial PR. Please take a look @NL66278

Copy link

@alexschubert alexschubert left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

During functional testing, the following error appeared when trying to add a session to a newly created event:

Uncaught Error: KeyError: 'id'
http://3349535-127-b590c5.runbot3.odoo-community.org/web/content/418-c35d703/web.assets_backend.js:369
Traceback:
Error: KeyError: 'id'
at F.getitem (http://3349535-127-b590c5.runbot3.odoo-community.org/web/content/418-c35d703/web.assets_backend.js:369:161)
at F.getattr (http://3349535-127-b590c5.runbot3.odoo-community.org/web/content/418-c35d703/web.assets_backend.js:370:69)
at F.getattribute (http://3349535-127-b590c5.runbot3.odoo-community.org/web/content/418-c35d703/web.assets_backend.js:203:38)
at Object.py.PY_getAttr (http://3349535-127-b590c5.runbot3.odoo-community.org/web/content/418-c35d703/web.assets_backend.js:167:176)
at Object.py.evaluate (http://3349535-127-b590c5.runbot3.odoo-community.org/web/content/418-c35d703/web.assets_backend.js:270:11)
at Object.py.evaluate (http://3349535-127-b590c5.runbot3.odoo-community.org/web/content/418-c35d703/web.assets_backend.js:265:111)
at Object.py.evaluate (http://3349535-127-b590c5.runbot3.odoo-community.org/web/content/418-c35d703/web.assets_backend.js:267:99)
at Object.py.eval (http://3349535-127-b590c5.runbot3.odoo-community.org/web/content/418-c35d703/web.assets_backend.js:272:284)
at http://3349535-127-b590c5.runbot3.odoo-community.org/web/content/418-c35d703/web.assets_backend.js:380:136
at Function..each..forEach (http://3349535-127-b590c5.runbot3.odoo-community.org/web/content/412-150dd49/web.assets_common.js:12:558)

Steps to reproduce:

  1. Open the "Events" module
  2. Click on the "Create" action
  3. Without editing the event any further, add a session to the "Email Schedule" notebook page
  4. Click on the Session dropdown
  5. The error appears

Apart from this error, I was able to validate the general functionality of the module in addition to features like separate session registration and seat availability per session.

@ntsirintanis
Copy link
Contributor Author

@alexschubert Thanks a lot for your review and the time spent testing the module. I have corrected the error by hiding event_mail_ids tree if there are no sessions associated to the event. This is different than 10.0, where one could actually create a session without having an event record. The way it is implemented now is more logical and clear in my opinion.

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Looks good (code and runbot). Just a minor comment:

event_session/views/event_session_view.xml Outdated Show resolved Hide resolved
@ntsirintanis
Copy link
Contributor Author

@chienandalu Thanks for your review and suggestion. Event_session has an option for pivot view now.

Copy link

@NL66278 NL66278 left a comment

Choose a reason for hiding this comment

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

Tested in practice and looks good to me.

@pedrobaeza pedrobaeza merged commit c1290e8 into OCA:11.0 Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants