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

[15.0][ADD] base_ical #231

Closed
wants to merge 7 commits into from
Closed

[15.0][ADD] base_ical #231

wants to merge 7 commits into from

Conversation

hbrunn
Copy link
Member

@hbrunn hbrunn commented Jul 10, 2023

This module allows administrators to configure iCalendars based on an arbitrary selection on arbitrary models.

Users can selectively subscribe to them by enabling them in their profile form.

This is useful for exposing Odoo data to calendaring application like Nextcloud.

Configuration

To configure this module, you need to:

  • Go to Settings/Technical/iCalendars

  • Create a calendar, fill in the model you want to expose and possibly a domain to restrict records. You can use the user variable to restrict things relative to the user using the calendar

  • A few iCalendar-fields have defaults that should work for any model, you'll have to fill in expressions manually though for the start and end date of the records.

    For example, for model calendar.event, you'd fill in record.allday and record.start_date or record.start as DTSTART and record.allday and record.stop_date or record.stop as DTEND.

    For model hr.leave, you'd write (record.request_unit_half or record.request_unit_hours) and record.date_from or record.date_from.date() for DTSTART and (record.request_unit_half or record.request_unit_hours) and record.date_to or (record.date_to.date() + timedelta(days=1)) for DTEND - this is a bit more complex because of the way Odoo handles the begin and end times of leaves, and you'll want the extra day as most clients interpret the end date as non-inclusive.

  • Existing calendars are available for users in the tab Calendars of their profile form, where they can enable them to obtain a link they can paste into whatever client they are going to use

Usage

To use this module, you need to:

  • Go to your profile form
  • Click Enable on one of the calendars listed in tab Calendars
  • Copy the URL to the application you use

@albig
Copy link

albig commented Jul 31, 2023

Thanks @hbrunn for this great PR!

I've done some functional testing and found the following issues:

  1. uninstall not possible:
  • AttributeError: module 'odoo.addons.base_ical' has no attribute 'uninstall_hook'
  1. ordinary users cannot access their profile anymore:
Access Error
×
The requested operation can not be completed due to security restrictions.

Document type: Users (res.users)
Operation: read
User: 24
Fields:
- address_home_id (allowed for groups 'Employees / Officer')
- barcode (allowed for groups 'Employees / Officer')
- birthday (allowed for groups 'Employees / Officer')
- certificate (allowed for groups 'Employees / Officer')
- children (allowed for groups 'Employees / Officer')
- country_of_birth (allowed for groups 'Employees / Officer')
- emergency_contact (allowed for groups 'Employees / Officer')
- emergency_phone (allowed for groups 'Employees / Officer')
- employee_bank_account_id (allowed for groups 'Employees / Officer')
- employee_country_id (allowed for groups 'Employees / Officer')
- employee_phone (allowed for groups 'Employees / Officer')
- gender (allowed for groups 'Employees / Officer')
- hours_last_month_display (allowed for groups 'Employees / Officer', 'Attendances / Officer')
- identification_id (allowed for groups 'Employees / Officer')
- km_home_work (allowed for groups 'Employees / Officer')
- marital (allowed for groups 'Employees / Officer')
- passport_id (allowed for groups 'Employees / Officer')
- permit_no (allowed for groups 'Employees / Officer')
- pin (allowed for groups 'Employees / Officer')
- place_of_birth (allowed for groups 'Employees / Officer')
- spouse_birthdate (allowed for groups 'Employees / Officer')
- spouse_complete_name (allowed for groups 'Employees / Officer')
- study_field (allowed for groups 'Employees / Officer')
- study_school (allowed for groups 'Employees / Officer')
- visa_expire (allowed for groups 'Employees / Officer')
- visa_no (allowed for groups 'Employees / Officer')

  1. I cannot find the URL. The field is empty (user has "personal / officer" rights):

2023-07-31_10-57_1

My configuration:

2023-07-31_10-57

@hbrunn
Copy link
Member Author

hbrunn commented Jul 31, 2023

1, 2: will fix that soon
3: did you click the "Enable" button? I try to make my code privacy friendly by default

@albig
Copy link

albig commented Jul 31, 2023

3: did you click the "Enable" button? I try to make my code privacy friendly by default

My fault. I didn't realized this "Enable" as button. A column description would be great. Or maybe better use a checkbox like in the other Odoo table listings?

2023-07-31_11-19

@albig
Copy link

albig commented Jul 31, 2023

Calling the provided URL is not yet possible for me:

odoo.exceptions.AccessError
odoo.exceptions.AccessError: Sie haben keinen Zugriff auf 'Datenmodelle' (ir.model) Datensätze.

Der Vorgang ist für die folgenden Gruppen erlaubt:
	- Administration/Access Rights

Wenden Sie sich gegebenenfalls an Ihren Administrator, um den Zugang zu beantragen.

Maybe releated to Point 2.

@hbrunn
Copy link
Member Author

hbrunn commented Jul 31, 2023

1 and the last access error are fixed, 2 I fail to reproduce on runboat (or locally), we'll have to discuss that later I think

@albig
Copy link

albig commented Jul 31, 2023

Very strange. Cannot reproduce this behaviour either now. Also ordinary users may access the profile and edit the ics-calendar records. The ics-export works now as expected. I didn't restart the Odoo process before maybe. May this be the reason? And I checked it only on my local system.

@albig
Copy link

albig commented Sep 15, 2023

On another instance, I have the same issue "2.)" from above after install of base_ical :-(

@albig
Copy link

albig commented Sep 18, 2023

The issue 2. is now fixed with your last commit.

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Mar 24, 2024
Copy link

@amh-mw amh-mw left a comment

Choose a reason for hiding this comment

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

Neat! This is a much more robust implementation of something I attempted myself in odoo/odoo#53986.

active = fields.Boolean(default=True)
ical_id = fields.Many2one("base.ical", required=True, ondelete="cascade")
user_id = fields.Many2one("res.users", required=True, ondelete="cascade")
token = fields.Char(required=True, default=lambda self: secrets.token_urlsafe())
Copy link

Choose a reason for hiding this comment

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

I'll raise the same argument here that I (correctly) lost in my own pull request (odoo/odoo#53986 (comment)), that it is a bad idea to introduce a new authentication mechanism, especially one that stores a text/plain token in the database. I ended up using a res.users.apikeys with a custom scope.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for this, very good point. Bad news is however that this needs quite some effort, and there's no further funding for this module at the moment.

I'll add this to my OCA-TODOs, but it goes to the end of the queue, so no ETA unfortunately

@@ -4,3 +4,4 @@ mysqlclient
pymssql<=2.2.5
sqlalchemy
sqlalchemy-hana
vobject
Copy link

Choose a reason for hiding this comment

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

This package requirement suggests that this module should land in https://github.com/OCA/calendar instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually don't mind very much where to put this, if other reviewers feel it should go there, I'll be happy to oblige.

Copy link
Member

Choose a reason for hiding this comment

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

vobject also offers vtodo if you want to support it (based on mail.activity) which works quite nicely if you use a html2text function for the description. This would open a new possibility and would go further than a way to sync calendar clients. This would mean that it shouldn't be in calendar if we make it possible.

Copy link

Choose a reason for hiding this comment

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

In my opinion it would be slightly easier to discover this module (by functional users) and be more intuitive in /OCA/calendar even though if it would support vtodo in the future.

@@ -0,0 +1,4 @@
The development of this module has been financially supported by:
Copy link

Choose a reason for hiding this comment

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

Delete this placeholder file?

preview = fields.Text(compute="_compute_preview")
expression_dtstamp = fields.Char(
required=True,
vevent_field="dtstamp",
Copy link

Choose a reason for hiding this comment

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

I am unfamiliar with this capability of Odoo fields. You're able to decorate the field with a custom vevent_field argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

you can add arbitrary arguments, you should however add it in _valid_field_parameter to avoid a warning. Very handy as this makes the code below much more concise

)
user_url = fields.Char(compute="_compute_user_fields", string="URL")
user_active = fields.Boolean(compute="_compute_user_fields")
auto = fields.Boolean(
Copy link

Choose a reason for hiding this comment

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

I feel like auto=True has too large a blast radius unless paired with some sort of group or base_user_role based restriction?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's why it's off by default. It is actually restricted to backend users below, so it depends very much on your specific case if it's a good idea to turn this on or not

from odoo import http


class MyController(http.Controller):
Copy link
Member

Choose a reason for hiding this comment

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

MyController should probably be renamed to something more specific.

Copy link
Member

@fkantelberg fkantelberg left a comment

Choose a reason for hiding this comment

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

I left some notes with a suggestion. If you like I could also spend some time to implement those and/or the swap to res.users.apikeys. Just comment if you would appreciate the suggestion/support and what you would like to see.

<field name="model" invisible="True" />
<field
name="domain"
placeholder="[('user_id', '=', user.id)]"
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to introduce the domain widget here?

Suggested change
placeholder="[('user_id', '=', user.id)]"
placeholder="[('user_id', '=', user.id)]"
widget="domain"
options="{'model': 'model'}

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd love to, but it can't handle expressions - that would need quite some work in the domain widget itself to make this work


For example, for model ``calendar.event``, you'd fill in ``record.allday and record.start_date or record.start`` as `DTSTART` and ``record.allday and record.stop_date or record.stop`` as `DTEND`.

For model ``hr.leave``, you'd write ``(record.request_unit_half or record.request_unit_hours) and record.date_from or record.date_from.date()`` for `DTSTART` and ``(record.request_unit_half or record.request_unit_hours) and record.date_to or (record.date_to.date() + timedelta(days=1))`` for `DTEND` - this is a bit more complex because of the way Odoo handles the begin and end times of leaves, and you'll want the extra day as most clients interpret the end date as non-inclusive.
Copy link
Member

Choose a reason for hiding this comment

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

This shows a bit of the disadvantage of the approach here because you need quite complex or ... and ... or constructs to achieve some stuff because you can't use variables in such snippets. For calendar.event it would be nice to use the core function _get_ics_file too. I would suggest to use a single multi-line snippet with a defined interface and use safe_eval in "exec" mode. The downside is that it's more complicated but more flexible and allows additional fields like DESCRIPTION, RRULE ...

# Also add vobject to the context
context["events"] = self._get_events()
safe_eval.safe_eval(self.code, context, mode="exec", nocopy=True)

# We could define that result can either be a list of dict or an iCalendar object
result = context.get("result", None)

if isinstance(result, list):
    calendar = vobject.iCalendar()
    for event in result:
        vobj = cal.add("vevent")
        for key, val in event.items():
            # TZ check for datetime etc.
            vobj.add(key).value = val
    return calendar.serialize()
return result

Snippets could be:

  • calendar.event: result = events._get_ics_file()
  • hr.leave:
result = []
for event in events:
    if event.request_unit_half or event.request_unit_hours:
        dtstart = event.date_from
        dtend = event.date_to
    else:
        dtstart = event.date_from.date()
        dtend = event.date_from.date()
    result.append(
        {
            "dtstart": dtstart,
            "dtend": dtend,
            "summary": event.name,
            "uid": ...,  # a bit unsure but I would just pass a function -> record to uid to the snippet
        }
    )

Funnily enough that would make it possible to generate vtodo based on mail.activity aswell.

Copy link
Member Author

Choose a reason for hiding this comment

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

can't we have both? As in (pseudo-python):

result = {
  'dtend': eval(expression_dtend),
  ...
}

if code_full:
  safe_eval(code_full, locals)
  result.update(**locals['result'])

and the field for code_full would go to base.group_noone or a dedicated one? Then for simple cases admins can use the fields, if you want to do anything fancy you switch to the full code mode.

I'm all for injecting some helper functions in the eval context, and vtodos, sure. I think I'd also implement this with a simple mode (radio for this is a vevent or vtodo, showing/hiding the appropriate expression fields) and the full code mode where you can overwrite all of the above.

Copy link
Member

Choose a reason for hiding this comment

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

Both might be complicated because the _get_ics_file are generating full calendars => I would run the snippet on multiple events instead of one event which would make merging both hard.

Maybe I can go with a switch for either simple or advanced mode but mixing is complex. In the end it boils down to the administrator to set it up. We could also just offer an advanced mode but some snippets for various basic cases.

@@ -4,3 +4,4 @@ mysqlclient
pymssql<=2.2.5
sqlalchemy
sqlalchemy-hana
vobject
Copy link
Member

Choose a reason for hiding this comment

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

vobject also offers vtodo if you want to support it (based on mail.activity) which works quite nicely if you use a html2text function for the description. This would open a new possibility and would go further than a way to sync calendar clients. This would mean that it shouldn't be in calendar if we make it possible.

if not token:
return http.request.not_found()
response = http.request.make_response(
token.ical_id.with_user(token.user_id)._get_ical(),
Copy link
Member

Choose a reason for hiding this comment

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

sudo() would be still active here which might work or might cause problems when the domain isn't defined correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

fortunately with_user resets the su flag - otherwise we'd see a lot of nasty privilege escalations out there

@hbrunn
Copy link
Member Author

hbrunn commented Apr 1, 2024

@fkantelberg if you've time and/or even funding for developing this further, please, by all means go ahead and do so. Just ping me on your PR, then I'll close this one, as I won't be able to work on this any time soon. My current OCA time itch is OCA/server-ux#849

@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 7, 2024
@jans23
Copy link

jans23 commented Apr 8, 2024

I tested it for model calendar.event as described in the README and created events in Odoo. The events were synchronized to Thunderbird successfully but didn't contain the reminder I setup in Odoo. Any idea?

@hbrunn
Copy link
Member Author

hbrunn commented Apr 8, 2024

I assume that would need this addon exporting VALARM components, which it doesn't. Should be easy to add though.

@amh-mw
Copy link

amh-mw commented May 6, 2024

@hbrunn Close this one in favor of #286?

@hbrunn hbrunn closed this May 6, 2024
SiesslPhillip pushed a commit to grueneerde/OCA-server-backend that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-backend (15.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants