Merge lp:~renatofilho/qtorganizer5-eds/fix-1445577 into lp:qtorganizer5-eds

[Renato Araujo Oliveira Filho]

Correct save events with UTC format.

[Renato Araujo Oliveira Filho (renatofilho)]

Reading the bug report I would say that the event is losing the timezone when edited. Is the app setting the correct timezone? If the app does not set the timezone the event will be handled as a Floating event.

review: Needs Information

[Gary.Wang (gary-wzl77)]

Actually, there is no such a timezone settings in app. App just follows system timezone.

My Time & Date in system settings
Time zone:
Asia/Shangehai UTC+8

----Create an event (display correctly in weeklyview)
qml: event.name: Ttttt
qml: event.startDateTime: Fri Jul 24 07:00:00 2015 GMT+0800
qml: event.endDateTime: Fri Jul 24 08:00:00 2015 GMT+0800

----Edit an event with only name changed (wrong display and event backs x hours)
qml: event.name: Ttttt-edit
qml: event.startDateTime: Fri Jul 24 07:00:00 2015 GMT+0800
qml: event.endDateTime: Fri Jul 24 08:00:00 2015 GMT+0800

[Gary.Wang (gary-wzl77)]

I check this issue today and find sth as following:
when user modify a event with start or end date time changed, issue wont' occur.
when user modify a event without start datetime and end date time changed, issue occurs.
Reason for this is if start and end date time are not changed when modification, the timeSpec is for these two datetime is Qt::TimeZone instead of Qt::UTC when parsing them, then
tz.id().constData() == "able/localtime" which cause timezone == null and empty for tzId after icaltimezone_get_builtin_timezone was called.
so finally event backs x(timezone) hours after e_cal_client_modify_objects called.

As I mentioned above, we follow system timezone and have no way to set timezone in qml.
The easy fix is if start & end datetime are not floating time, then we need to convert Local timezone to UTC time zone.
Please review latest MP.
Thanks

[Renato Araujo Oliveira Filho (renatofilho)]

Could you resubmit this MR adding "https://code.launchpad.net/~renatofilho/qtorganizer5-eds/fix-1437305/+merge/261666" as Prerequisite Branch, it will make only your changes visible on the diff. And make it easy to review.

[Gary.Wang (gary-wzl77)]

OK, I will delete this one later on and create another MP for review since I can't resubmit merge proposal in same branch.
Thanks.

[Renato Araujo Oliveira Filho (renatofilho)]

Could you write a small qml code, that reproduce the error?

We can not save all events with 'UTC' timezone it will cause the alarms to fail, we need to have a way to save events without a specific TZ to floating times.

If js does not support datetime with 'Qt::TimeZone' format we should fix the QML bindings to return it in UTC format. (this need to be done in QtOrganizer)

As workaround we can update the start/stop field before save the event (in calendar app) even if the user does not change it.

But first I need to see the small qml app to make sure that we do not have any other solution.

[Gary.Wang (gary-wzl77)]

This issue can be reproduce easily. Just make sure you have non UTC+0 time
zone on your device. e.g. for me UTC+8
Please find simple qml app here
https://code.launchpad.net/~gary-wzl77/+junk/testtz

Regarding workaround, I've tried add one milliseconds for start/end
datetime before when editing an event, it *works*. But strictly speaking, I
think it's just a workaround not a proper fix.

I have no idea beside eds, if there's another plugins for QtOrganizer(what
kind of plugin use by Jolla)? if it works fine with another plugin, I am
afraid issue lies in eds.
Also applying 'UTC' timezone for all events cause alarm(floating time) to
fail, no idea if there is a way to check if edit item type or sth.

Please try the qml code at first to have a check if it can be reproduced on
your device, Thanks.

It seems we can't check current event type is alarms or

On Tue, Sep 8, 2015 at 9:39 PM, Renato Araujo Oliveira Filho <
<email address hidden>> wrote:

> Could you write a small qml code, that reproduce the error?
>
>
> We can not save all events with 'UTC' timezone it will cause the alarms to
> fail, we need to have a way to save events without a specific TZ to
> floating times.
>
> If js does not support datetime with 'Qt::TimeZone' format we should fix
> the QML bindings to return it in UTC format. (this need to be done in
> QtOrganizer)
>
> As workaround we can update the start/stop field before save the event (in
> calendar app) even if the user does not change it.
>
> But first I need to see the small qml app to make sure that we do not have
> any other solution.
>
> --
>
> https://code.launchpad.net/~gary-wzl77/qtorganizer5-eds/fix_1445577/+merge/269390
> You are the owner of lp:~gary-wzl77/qtorganizer5-eds/fix_1445577.
>

--
Br
Gary.Wzl

[Renato Araujo Oliveira Filho (renatofilho)]

I am trying to reproduce the bug with the example create by Gary. But the log is showing the correct results.

qml: -------------Create event
qml: Birthdays_145 ---- false
qml: Personal ---- false
qml: renatoteste2gmailcom_145 ---- false
qml: HolidaysinBrazil_145 ---- false
qml: Personal ---- false
qml: Personal ---- true
qml: Birthdays & Anniversaries ---- true
qml: event Name:Test_1443797923448
qml: startDate: sex out 2 11:58:43 2015 GMT-0300
qml: endDate: sex out 2 12:58:43 2015 GMT-0300
qml: -------------Create event done
qml: --------onModelChanged
qml: editEvent: QDeclarativeOrganizerEvent(0x2e8d020)
qml: event Name:Test_1443797923448
qml: startDate: sex out 2 11:58:43 2015 GMT-0300
qml: endDate: sex out 2 12:58:43 2015 GMT-0300
qml: --------onModelChanged
qml:
-------------Edit event
qml: event Name:edit___Test_1443797923448
qml: startDate: sex out 2 11:58:43 2015 GMT-0300
qml: endDate: sex out 2 12:58:43 2015 GMT-0300
qml: -------------Edit event done
qml: --------onModelChanged
qml: editEvent: QDeclarativeOrganizerEvent(0x2e8d020)
qml: event Name:edit___Test_1443797923448
qml: startDate: sex out 2 11:58:43 2015 GMT-0300
qml: endDate: sex out 2 12:58:43 2015 GMT-0300
qml: --------onModelChanged

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:88
http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-ci/232/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-amd64-ci/39/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/39
        deb: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/39/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-i386-ci/39

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtorganizer5-eds-ci/232/rebuild

review: Needs Fixing (continuous-integration)

[Gary.Wang (gary-wzl77)]

I have a quick testing on your latest MR.
I see you have a UTC checker when saving an event.
It fixes this problem.
Thanks.

PS: I see you can't reproduce this bug on your side from the comment.I think it's because you have UTC+0 timezone for date/time in your system-setting, If you set non-UTC timezone, it can be reproduced 100%.

[lp:~renatofilho/qtorganizer5-eds/fix-1445577]

89. By Renato Araujo Oliveira Filho on 2015-10-28

Trunk merged.

90. By Renato Araujo Oliveira Filho on 2015-10-28

Updated changelog.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:89
http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-ci/233/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-amd64-ci/40
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/40
        deb: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/40/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-i386-ci/40/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtorganizer5-eds-ci/233/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:90
http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-ci/234/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-amd64-ci/41/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/41
        deb: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/41/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-i386-ci/41/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtorganizer5-eds-ci/234/rebuild

review: Needs Fixing (continuous-integration)

[lp:~renatofilho/qtorganizer5-eds/fix-1445577]

91. By Renato Araujo Oliveira Filho on 2015-10-28

Run tests sequential.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:91
http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-ci/235/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-amd64-ci/42
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/42
        deb: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/42/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-i386-ci/42

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtorganizer5-eds-ci/235/rebuild

review: Approve (continuous-integration)

[lp:~renatofilho/qtorganizer5-eds/fix-1445577]

92. By Renato Araujo Oliveira Filho on 2015-10-29

Create unit test for events saved on UTC timezone.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:92
http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-ci/236/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-amd64-ci/43/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/43/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-i386-ci/43/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtorganizer5-eds-ci/236/rebuild

review: Needs Fixing (continuous-integration)

[lp:~renatofilho/qtorganizer5-eds/fix-1445577]

93. By Renato Araujo Oliveira Filho on 2015-10-29

Trunk merged.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:93
http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-ci/237/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-amd64-ci/44
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/44
        deb: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/44/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-i386-ci/44

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtorganizer5-eds-ci/237/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

lgtm.

review: Approve

=== modified file 'debian/rules'
--- debian/rules	2013-08-15 20:50:06 +0000
+++ debian/rules	2015-10-29 17:02:24 +0000
@@ -10,3 +10,6 @@
 
 override_dh_auto_configure:
 	dh_auto_configure -- -DCMAKE_INSTALL_LIBEXECDIR=/usr/lib/$(DEB_HOST_MULTIARCH)/address-book-service
+
+override_dh_auto_test:
+	ctest -j1 --output-on-failure

=== modified file 'organizer/qorganizer-eds-engine.cpp'
--- organizer/qorganizer-eds-engine.cpp	2015-10-29 17:02:24 +0000
+++ organizer/qorganizer-eds-engine.cpp	2015-10-29 17:02:24 +0000
@@ -1211,24 +1211,33 @@
 QDateTime QOrganizerEDSEngine::fromIcalTime(struct icaltimetype value, const char *tzId)
 {
     uint tmTime;
+    bool allDayEvent = icaltime_is_date(value);
 
     // check if ialtimetype contais a time and timezone
-    if (!icaltime_is_date(value) && tzId) {
+    if (!allDayEvent && tzId) {
+        QByteArray tzLocationName;
         icaltimezone *timezone = icaltimezone_get_builtin_timezone_from_tzid(tzId);
-        // fallback: sometimes the tzId contains the location name
-        if (!timezone) {
-            timezone = icaltimezone_get_builtin_timezone(tzId);
+
+        if (icaltime_is_utc(value)) {
+            tzLocationName = "UTC";
+        } else {
+            // fallback: sometimes the tzId contains the location name
+            if (!timezone) {
+                timezone = icaltimezone_get_builtin_timezone(tzId);
+            }
+            tzLocationName = QByteArray(icaltimezone_get_location(timezone));
         }
+
         tmTime = icaltime_as_timet_with_zone(value, timezone);
-        QByteArray tzLocationName(icaltimezone_get_location(timezone));
         QTimeZone qTz(tzLocationName);
         return QDateTime::fromTime_t(tmTime, qTz);
     } else {
         tmTime = icaltime_as_timet(value);
         QDateTime t = QDateTime::fromTime_t(tmTime).toUTC();
-        // floating time contains invalid timezone
+        // all day or floating time events will be saved with invalid timezone
         return QDateTime(t.date(),
-                         (icaltime_is_date(value) ? QTime() : t.time()),
+                         // if the event is all day event save with emtpy time
+                         (allDayEvent ? QTime() : t.time()),
                          QTimeZone());
     }
 }

=== modified file 'tests/unittest/event-test.cpp'
--- tests/unittest/event-test.cpp	2015-10-29 17:02:24 +0000
+++ tests/unittest/event-test.cpp	2015-10-29 17:02:24 +0000
@@ -901,6 +901,44 @@
         QVERIFY(vcard.contains("TRIGGER;VALUE=DURATION;RELATED=START:-PT1M"));
     }
 
+    // BUG: #1445577
+    void testUTCEvent()
+    {
+        static QString displayLabelValue = QStringLiteral("UTC event");
+        static QString descriptionValue = QStringLiteral("UTC event");
+        const QDateTime startDate(QDateTime::currentDateTime().toUTC());
+
+        QOrganizerEvent event;
+        event.setStartDateTime(startDate);
+        event.setDisplayLabel(displayLabelValue);
+        event.setDescription(descriptionValue);
+
+        QtOrganizer::QOrganizerManager::Error error;
+        QMap<int, QtOrganizer::QOrganizerManager::Error> errorMap;
+        QList<QOrganizerItem> items;
+        QSignalSpy createdItem(m_engine, SIGNAL(itemsAdded(QList<QOrganizerItemId>)));
+        items << event;
+        bool saveResult = m_engine->saveItems(&items,
+                                              QList<QtOrganizer::QOrganizerItemDetail::DetailType>(),
+                                              &errorMap,
+                                              &error);
+        QTRY_COMPARE(createdItem.count(), 1);
+        QVERIFY(saveResult);
+
+        QList<QOrganizerItemId> ids;
+        QOrganizerItemFetchHint hint;
+        ids << items[0].id();
+        QList<QOrganizerItem> newItems = m_engine->items(ids, hint, &errorMap, &error);
+        QCOMPARE(newItems.size(), 1);
+
+        QOrganizerEvent newEvent = static_cast<QOrganizerEvent>(newItems[0]);
+        QCOMPARE(newEvent.startDateTime().timeZoneAbbreviation(), QStringLiteral("UTC"));
+        QCOMPARE(newEvent.startDateTime().date(), startDate.date());
+        QCOMPARE(newEvent.startDateTime().time().hour(), startDate.time().hour());
+        QCOMPARE(newEvent.startDateTime().time().minute(), startDate.time().minute());
+        QCOMPARE(newEvent.startDateTime().time().second(), startDate.time().second());
+    }
+
     void testExtendedProperties()
     {
         static QString displayLabelValue = QStringLiteral("event with extended property");