Enhance iCal file exports #162

rtnpro · 2015-01-28T18:36:49Z

This branch improves export of iCal files in the following ways:

Allow export iCal file for a calendar event
Add reminder logic in iCal file
when downloading iCal info for a meeting, allow the option to add a reminder as well.

NOTE: This is work in progress and the PR has been created for review of ongoing work.

pypingou · 2015-01-29T07:07:05Z

This is a nice start.
Had you kept #161 open I would have asked if you could adjust the unit-tests and I would have merged it, now I have to wait ;-)

rtnpro · 2015-01-29T07:17:05Z

@pypingou I can reopen #161 then, and add unit tests in it :)

pypingou · 2015-01-29T08:27:26Z

@rtnpro sure, works for me :)

rtnpro · 2015-02-20T18:57:04Z

@pypingou Could you review it?

Changes:

Add reminders in iCal file
Allow option in meeting snippet to export meeting ical with/without reminders
Allow configuring ical reminders from config file

rtnpro · 2015-02-20T18:57:37Z

@pypingou if the above is OK, I will add tests for the same.

pypingou · 2015-02-21T14:32:23Z

fedocal.cfg.sample

@@ -59,3 +59,6 @@ APP_URL = 'https://apps.fedoraproject.org/calendar/'
 ### In case of exception flask sends an email with some information to debug the
 ### problem. The email is sent to the address listed here.
 # EMAIL_ERROR = 'pingou@pingoured.fr'
+
+ICAL_REMINDERS = [{'days': -1}, {'hours': -1}, {'minutes': -5}]


We probably want to sync this with the default configuration and we should document a little more in the sample configuration

Changes summary: - 'ICAL_REMINDERS' in conf file replaced with 'ICAL_REMINDER_OPTIONS' for displaying dropdown menu for selecting reminder time when downloading a meeting's iCal file with a reminder. - Change in GET param: 'reminder' to 'reminder_at' for exporting iCal file for a meeting. - Change in 'add_meeting_to_vcal' API to accept no or single reminder rather than a list of reminders.

rtnpro · 2015-03-02T19:29:20Z

@pypingou Please review rtnpro@c99762f. Please have a look at the commit summary for a summary of the changes made.

…nt handler across modal reloads.

rtnpro · 2015-03-03T13:52:10Z

@pypingou True! Currently the flow would allow setting reminders after the meeting if the user toys around the URL :) However, I wanted to keep the get_timestamp_from_str generic and wanted to have the policy: reminder can be set before the event in the view layer. However, values like: 5 for 5 minutes, 60 for 1 hour and 60*24=1440 for 1 day seems simple and better. However, I am not able to think of a nice get parameter name for it. The param should explicitly mention the reminder is set at a time before the meeting. reminder_at needed - sign to indicate it. reminder_before won't need any sign before its value as the name itself says it all. But, I don't find the name reminder_before nice :. What do you say?

'reminder_delta=<minutes before meeting>' instead of 'reminder_at=<-?(?:\d+h)?(?:\d+m)?(?:\d+s)?>'. This leads to a simple way (int(reminder_delta)) to extract minutes before event info for setting reminder in the iCal file.

pypingou · 2015-03-04T21:52:47Z

fedocal/__init__.py

@@ -1030,7 +1038,9 @@ def view_meeting_page(meeting_id, full):
        tzone=tzone,
        title=meeting.meeting_name,
        editor=editor,
-        from_date=from_date)
+        from_date=from_date,
+        reminder_at_options=APP.config.get('ICAL_REMINDER_AT_OPTIONS') or []


No need for the or here, just finish the get:
reminder_at_options=APP.config.get('ICAL_REMINDER_AT_OPTIONS', [])
The second parameter is the default to return if the key is not in the dict ;-)

@pypingou I know, but
reminder_at_options=APP.config.get('ICAL_REMINDER_AT_OPTIONS') or []
handles the worst case scenario when

config[ICAL_REMINDER_AT_OPTIONS'] = None

It's unnecessary here. It's a side effect of writing fault tolerant code for E Commerce products to handle stupidity from some other sources ;). Here, it's unnecessary, I believe. I will fix it.

We shouldn't be able to have
config[ICAL_REMINDER_AT_OPTIONS'] = None
as it would mean someone edited the default_config.py instead of overriding it.

So +1 on not being tolerant to this, but in the worst case, I think I would prefer a if reminder_at_options lower down the stack.

…is False.

rtnpro · 2015-03-06T18:52:57Z

@pypingou I have added tests for this feature enhancement. Please review it.

pypingou · 2015-03-17T12:13:39Z

fedocal/templates/default/view_meeting.html

@@ -54,10 +54,25 @@ <h2 class="orange">{{ _('Meeting "%(name)s"', name=meeting.meeting_name) }}</h2>
    {% endmacro %}
    <p>{{ _('Meeting organized by %(managers)s', managers=get_managers(meeting.meeting_manager)) }}</p>

-    <a href="{{ url_for('ical_calendar_meeting', meeting_id=meeting.meeting_id) }}"
+    <a id="ical-meeting-export" href="{{ url_for('ical_calendar_meeting', meeting_id=meeting.meeting_id) }}"


Might be good to place the id on the line below to keep (more or less) the 80 char rule :)

pypingou · 2015-03-17T12:16:58Z

This is looking very nice and working nicely as well locally.

Two questions still and one remark: the tests are currently failing for me here :)

FAIL: Test the view_meeting_page function.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pingou/repos/gitrepo/fedocal/tests/test_flask.py", line 494, in test_view_meeting_page
    in output.data)
AssertionError: False is not true

rtnpro · 2015-03-18T11:43:03Z

@pypingou I am looking into the above, and I will fix it ASAP.

- Test for non-numeric values of reminder_delta GET param when exporting ical file for a meeting. - Test for a reminder_delta value not in default choices for reminder_delta.

rtnpro · 2015-03-28T09:36:34Z

@pypingou I have moved the refactor related to static js files in another branch. I have also added the remainder tests that you'd mentioned. Please review it and let me know if it's ready for merge.

Please note that JS issues related to ical reminder UI elements persist for meeting details page. This has been already fixed in the branch for JS refactor.

pypingou · 2015-03-28T10:09:54Z

This is looking really good!

I'm just going to wait for jenkins to kick in and then merge.

Enhance iCal file exports

ralphbean · 2015-03-28T11:43:28Z

Hooray!

🏯 This was epic. 🏰

Allow adding reminders for a meeting in a iCal file.

c087907

rtnpro force-pushed the ical_enhancement branch from d9e3f6c to c087907 Compare February 20, 2015 15:33

rtnpro added 2 commits February 21, 2015 00:00

Allow exporting a iCal file with reminders.

3dcb288

Make meeting iCal reminders configurable from config file.

04d7da2

pypingou reviewed Feb 21, 2015
View reviewed changes

BUGFIX: Retain iCal reminder checkbox & reminder options's change eve…

2e87275

…nt handler across modal reloads.

rtnpro force-pushed the ical_enhancement branch from 1adca0f to fb7f3e2 Compare March 4, 2015 19:23

pypingou reviewed Mar 4, 2015
View reviewed changes

rtnpro added 2 commits March 6, 2015 18:56

Hide iCal export option with reminder if bool(ICAL_REMINDER_OPTIONS) …

a24c6e1

…is False.

Added tests for reminder integration with iCal export.

b7fcaa2

pypingou reviewed Mar 17, 2015
View reviewed changes

rtnpro added 2 commits March 28, 2015 15:00

Added more tests for ical reminder integration.

b61d330

- Test for non-numeric values of reminder_delta GET param when exporting ical file for a meeting. - Test for a reminder_delta value not in default choices for reminder_delta.

Refactored view_meeting template for readability.

b3ec5c7

rtnpro force-pushed the ical_enhancement branch from 3eadb40 to b3ec5c7 Compare March 28, 2015 09:33

pypingou added a commit that referenced this pull request Mar 28, 2015

Merge pull request #162 from rtnpro/ical_enhancement

aca7a56

Enhance iCal file exports

pypingou merged commit aca7a56 into fedora-infra:master Mar 28, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance iCal file exports #162

Enhance iCal file exports #162

rtnpro commented Jan 28, 2015

pypingou commented Jan 29, 2015

rtnpro commented Jan 29, 2015

pypingou commented Jan 29, 2015

rtnpro commented Feb 20, 2015

rtnpro commented Feb 20, 2015

pypingou Feb 21, 2015

rtnpro commented Mar 2, 2015

rtnpro commented Mar 3, 2015

pypingou Mar 4, 2015

rtnpro Mar 6, 2015

pypingou Mar 6, 2015

rtnpro commented Mar 6, 2015

pypingou Mar 17, 2015

rtnpro Mar 18, 2015

pypingou commented Mar 17, 2015

rtnpro commented Mar 18, 2015

rtnpro commented Mar 28, 2015

pypingou commented Mar 28, 2015

ralphbean commented Mar 28, 2015

Enhance iCal file exports #162

Enhance iCal file exports #162

Conversation

rtnpro commented Jan 28, 2015

pypingou commented Jan 29, 2015

rtnpro commented Jan 29, 2015

pypingou commented Jan 29, 2015

rtnpro commented Feb 20, 2015

rtnpro commented Feb 20, 2015

pypingou Feb 21, 2015

Choose a reason for hiding this comment

rtnpro commented Mar 2, 2015

rtnpro commented Mar 3, 2015

pypingou Mar 4, 2015

Choose a reason for hiding this comment

rtnpro Mar 6, 2015

Choose a reason for hiding this comment

pypingou Mar 6, 2015

Choose a reason for hiding this comment

rtnpro commented Mar 6, 2015

pypingou Mar 17, 2015

Choose a reason for hiding this comment

rtnpro Mar 18, 2015

Choose a reason for hiding this comment

pypingou commented Mar 17, 2015

rtnpro commented Mar 18, 2015

rtnpro commented Mar 28, 2015

pypingou commented Mar 28, 2015

ralphbean commented Mar 28, 2015