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

Add ical subscription #2895

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Conversation

david-venhoff
Copy link
Member

@david-venhoff david-venhoff commented Jul 8, 2024

Short description

This pr implements support for automatically creating events from an icalendar. This only implements step 1 of issue #100, as it is not currently possible to automatically use machine translation with this feature and we need another pr for that. However, it is possible to manually start machine translation for imported events.

For now, this feature is only visible to service team members and cms team members.

To test this pr, you could use this ical url, which includes events for july from Saarburg: https://www.saarburg.de/?post_type=tribe_events&tribe-bar-date=2024-07-08&ical=1&eventDisplay=list

Proposed changes

  • Add a new model for external calendars, which contain an url to the ical file
  • Add fields to the event model to specify whether this event was automatically imported, and link it to the icalendar
  • Implement functionality for converting icalendar events into cms events and event translations
  • Add UI for managing external calendars

Side effects

  • We need a new cronjob that runs the import events command every once in a while

Resolved issues

Fixes: #100


Pull Request Review Guidelines

Copy link

codeclimate bot commented Jul 8, 2024

Code Climate has analyzed commit db83671 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 77.8% (50% is the threshold).

This pull request will bring the total coverage in the repository to 83.0% (-0.2% change).

View more on Code Climate.

@JoeyStk JoeyStk requested a review from deen13 July 8, 2024 11:32
@JoeyStk JoeyStk added ⛔ blocked Blocked by external dependency and removed ⛔ blocked Blocked by external dependency labels Jul 8, 2024
@JoeyStk
Copy link
Contributor

JoeyStk commented Jul 10, 2024

#2850 is merged now :)

@david-venhoff david-venhoff marked this pull request as ready for review July 16, 2024 10:21
@david-venhoff
Copy link
Member Author

Both blockers are now resolved :)

david-venhoff and others added 3 commits July 17, 2024 11:16
* Add an external calendar model
* Add functionality to import events from the url of external calendars

Co-authored-by: JoeyStk <72705147+JoeyStk@users.noreply.github.com>
@JoeyStk
Copy link
Contributor

JoeyStk commented Jul 18, 2024

Just two side notes: 1. This PR doesn't contain a release note, as we want to release this to the service team first without letting municipalities know about it (yet), 2. We want to do (1) so we test this very thoroughly and make sure everything works very smoothly before we let municipalities know about it. This makes is hopefully less daunting to review this PR :)

Copy link
Member

@MizukiTemma MizukiTemma left a comment

Choose a reason for hiding this comment

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

Thank you so much 💪 It looks a good start 😻

I have some suggestions but feel free to discard (or postpone if it is subject to planned extensions of the next step(s)).

  1. Fixing width of the columns in the external calendar list (see screen shots below)
    We are getting this kind of fix requests time to time. Maybe better to implement it already now?

  2. Checking the format of input in the field "URL"
    We have format check for the URL field of the model Organization. It probably makes sense here too?

  3. Warning when the URL or tag of a calendar is about to be changed
    Events that are imported from the calendar and already saved will be deleted after changing those fields, if they do not match the URL and tag anymore. In my opinion warning before saving makes the system less confusing and safe.

External calendar 1
External calendar 2

Copy link
Collaborator

@deen13 deen13 left a comment

Choose a reason for hiding this comment

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

First off, great job on this pull request! Your effort in tackling these issues is much appreciated. 🥳

General thoughts on the feature

  • Prefetch Existing Tags: If possible, it might be useful to fetch all tags once the domain is entered. This way, tags could be presented as a combobox instead of a text field, which could help prevent errors.
  • Import Error: There is an error occurring when importing events from Saarburg. The specific error is: Aug 01 08:46:13 ERROR integreat_cms.core.storages - An error occurred while importing events from this external calendar and Could not import 'Ausstellung Günter Schuster: Material und Sprache': The maximum duration for events is 28 days. Consider using recurring events if the event is not continuous.
  • Django Cron: Pull Request Store delivered push notification count per region and language #2920 introduces django_cron, which could be beneficial for our use case.
  • Event Review: It would be helpful to have an overview of all imported and erroneous events after import and before storing them. This would facilitate a thorough review process.
  • Error Handling: The status column provides a detailed explanation of errors, but it's unclear how to handle them. Consider adding a popup for errors to provide more space if there are multiple issues.
  • Error Management: Adding a button to clear reviewed errors would be useful so that subsequent users do not have to deal with errors that have already been addressed. This would also aid in identifying new errors during cron job polling.
  • Security Considerations: Should we implement some form of verification for this feature to prevent domain takeover and malicious event injection? More on domain takeover vulnerabilities

@david-venhoff
Copy link
Member Author

Thanks for your review!

Prefetch Existing Tags

This seems like a nice QoL improvement. However, that would not be a part of this pr, since we currently just intend to provide an mvp for the service team to test this feature. If everything works out, we will build on this mvp later.

Import error

I don't think there is much we can do about that. It has been a design decision to not allow events longer than 28 days in our system and the external calendar contains an event that is longer than that.

Django Cron

I didn't have a look at the pr yet, but that could be a nice alternative :)

Event Review

This could be a separate issue that we implement later

Error Handling

Same here

Error Management

Same here

Security Considerations

What is the threat you are thinking of? Are you worried that municipalities redirect to another provider for providing their icalendars? Because then I think it would be up to the municipalities to make sure that there are no security issues.
Or that municipalities enter the url of a third-party website, which might be malicious? That does not seem like a domain takeover vulnerability to me and also seems like it would be the responsibility of the municipalities to me.

@JoeyStk
Copy link
Contributor

JoeyStk commented Aug 5, 2024

Warning when the URL or tag of a calendar is about to be changed
Events that are imported from the calendar and already saved will be deleted after changing those fields, if they do not match the URL and tag anymore. In my opinion warning before saving makes the system less confusing and safe.

I think this also would probably be something for a new issue :)

@JoeyStk JoeyStk force-pushed the feature/add-ical-subscription branch from 652fca0 to db83671 Compare August 5, 2024 09:54
@MizukiTemma
Copy link
Member

Warning when the URL or tag of a calendar is about to be changed
Events that are imported from the calendar and already saved will be deleted after changing those fields, if they do not match the URL and tag anymore. In my opinion warning before saving makes the system less confusing and safe.

I think this also would probably be something for a new issue :)

Ok 😉 Will you open an issue for that?

@JoeyStk JoeyStk mentioned this pull request Aug 5, 2024
3 tasks
@JoeyStk
Copy link
Contributor

JoeyStk commented Aug 5, 2024

Ok 😉 Will you open an issue for that?

Sure! It's the first comment in here :)

Copy link
Member

@MizukiTemma MizukiTemma left a comment

Choose a reason for hiding this comment

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

Thank you so much, looks good to me 😸 Let's merge and try 🚀

💡 Don't forget to squash, please.

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.

Events: iCal subscription
4 participants