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

[16.0][MIG] event_session: Migration to version 16.0 #325

Merged
merged 21 commits into from
Jul 11, 2023

Conversation

stefan-tecnativa
Copy link
Contributor

cc @Tecnativa TT43670

@chienandalu @CarlosRoca13 please review!

event_session/models/event_registration.py Outdated Show resolved Hide resolved
event_session/models/event_session.py Outdated Show resolved Hide resolved
@stefan-tecnativa stefan-tecnativa force-pushed the 16.0-mig-event_session branch 2 times, most recently from a2ed343 to 8a764db Compare June 30, 2023 13:21
event_session/models/event_event.py Outdated Show resolved Hide resolved
event_session/views/event_session.xml Outdated Show resolved Hide resolved
event_session/views/event_session.xml Outdated Show resolved Hide resolved
event_session/models/event_event.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the work done!

I did a first quick review, but I'll do a second pass asap

event_session/models/event_session.py Outdated Show resolved Hide resolved
event_session/tests/test_event_session.py Outdated Show resolved Hide resolved
event_session/tests/test_event_session.py Outdated Show resolved Hide resolved
event_session/tests/test_event_session.py Outdated Show resolved Hide resolved
event_session/models/event_session.py Outdated Show resolved Hide resolved
@chienandalu
Copy link
Member

@stefan-tecnativa please include #328 once is merged

@stefan-tecnativa stefan-tecnativa force-pushed the 16.0-mig-event_session branch 2 times, most recently from 9c8ab0a to 9ffce55 Compare July 6, 2023 06:07
chienandalu and others added 19 commits July 6, 2023 08:20
* 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
The big issue with this kind of code is that if you have a user with a
certain locale being used, it will override the locale system wide and
the next request on the server may have the dates formatted with
strftime in a different language than expected...

The other issue is that some systems do not provide a set of actual
locales so on a server running in docker without manually modifying
a dockerfile to install a set of locales, it may fail to switch
the locale to a non existing one. It will just fail to compute the field.

Babel is already a dependency of odoo and it's a bit strange to not use it.

Get the locale the same way ir.qweb.fields do

This commit use the same method as ir.qweb.fields to get the locale
to use to translate a date. The previous code didn't took into account
a case where the lang is undefined and this case also use the same
default language as defined in odoo en_US.

As a result, the behaviour of event_session is more in line with
the current behaviour in odoo which is to default to en_US if locale
is undefined. With the global locale setting, the default locale
would be the system one so it may give different result or strange
behaviours or simply prevent correctly working without manual
intervention on the host.. While babel is already supported and used
by odoo.

Modified the test to actually test correctly the date formatted

As the default lang is en_US the format of a date is the following:

- month/day/year

And time format is on 12 hours so not 22:00 to 23:00 but 10:00 PM to 11:00 PM

With those changes, setting the locale to en_UK,en_CA will render the correct date using

- day/month/year and hours on 24h.

Added tests to confirm the translations work correctly on some locales

Check case when locale=None should default to en_US
Check that lang change is correctly taken into account.
Check that it works with languages returning unicode text (Russian)
When we create a session with seats_max = 0, we get an error when we try
to view it.

Doing this changes we achieve to avoid None values on the fields
seats_available and seats_available_pc
Attendee count is failing because records are being accumulated without taking into account possible changes in event.registration

With these changes, it is possible to correctly calculate the count of attendees.

TT29930
* Dropped the event_mail dependency, make it work with core mail schedulers instead.
* Support core voucher / badge reports for sessions.
* Now event.session inherits from event.event (like product.product and product.template)
* Make it easier to add more sessions to an event (refactored wizard, using recurrency-like rules)
* Play nice with regular events (events that don't use sessions) -- it's possible to have both kinds at the same time
* Several UX improvements
* Added more tests cases
@chienandalu
Copy link
Member

@stefan-tecnativa please include #328 once is merged

Already merged

@stefan-tecnativa stefan-tecnativa force-pushed the 16.0-mig-event_session branch 2 times, most recently from 57d2e96 to 1d031fc Compare July 6, 2023 09:22
@ivantodorovich
Copy link
Contributor

Without the "SAVE" button on form views, this mechanism to write on multiple sessions at once is a bit weird.
I feel it'll be confusing for the user to know what he's modifying because he doesn't know when the record is gonna be saved.

image

This mechanism was inspired by a simlar Odoo core feature in recurring Calendar Events. I checked to see how Odoo dealt with the dissapearence of the "SAVE" button there, and it seems nothing changed

image

Althuogh it's more commonly modified from the calendar view, where there's the "SAVE" button, so I guess they just don't care / didn't notice it

image


I'm not sure what the best thing to do is here, options are:

  • A. Keep it as-is
  • B. Deprecate / remove this feature
  • C. Find a better way, in terms of UX

Opinions?

ping @gregory-moka

Copy link
Contributor

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Apart from the topic mentioned above, I did a code and functional review and it all seems good ! Nice work!

event_session/models/event_event.py Outdated Show resolved Hide resolved
event_session/models/event_registration.py Outdated Show resolved Hide resolved
event_session/models/event_registration.py Outdated Show resolved Hide resolved
@chienandalu
Copy link
Member

Without the "SAVE" button on form views, this mechanism to write on multiple sessions at once is a bit weird. I feel it'll be confusing for the user to know what he's modifying because he doesn't know when the record is gonna be saved.
I'm not sure what the best thing to do is here, options are:

A. Keep it as-is
B. Deprecate / remove this feature
C. Find a better way, in terms of UX

I'd keep it, and a better way to handle UX could be to have those options folded by default (using bootstrap magic) so the user that knows what's doing can take advantage of them.

@stefan-tecnativa
Copy link
Contributor Author

First of all thanks you @ivantodorovich and @chienandalu for your time and suggestions. About what you were discussing, About what you were discussing, I feel that right now I can't give an opinion, so I prefer to let the experts comment 😉

@chienandalu
Copy link
Member

About what you were discussing, I feel that right now I can't give an opinion, so I prefer to let the experts comment wink

Ok, summarize the discussion in the roadmap

Removed test_event_event_form because now date_begin and date_end are
filled when the event is created, so the test case no longer made sense.
@stefan-tecnativa
Copy link
Contributor Author

About what you were discussing, I feel that right now I can't give an opinion, so I prefer to let the experts comment wink

Ok, summarize the discussion in the roadmap

Understood

@pedrobaeza
Copy link
Member

/ocabot merge nobump
/ocabot migration event_session

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-325-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jul 11, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Jul 5, 2023
15 tasks
@OCA-git-bot OCA-git-bot merged commit c268af5 into OCA:16.0 Jul 11, 2023
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at efa279b. Thanks a lot for contributing to OCA. ❤️

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.