1546606 - Refactor caldav request handling

Status: RESOLVED FIXED

(Calendar :: Internal Components, enhancement)

Description

[Philipp Kewisch [:Fallen][📱📆]]

I've been meaning to finalize this patch, the past few times I've looked I'm not actually sure what was missing. Before I keep moving this up I am going to dump it here. Maybe it makes sense to have someone with a fresh pair of eyes look at it. Magnus, can you find someone to work on this?

If you want to rebase this locally instead of applying it you can do:

hg up -r c7cda013b0b433778d7f9a64b12823ab6c40f01c
hg import http://...
hg rebase -d comm

Comments:

• The tests make sense to me, but Geoff had once noted the gdata tests are hard to read so maybe these tests need more documentation or changes.
• I'm not sure where it went, but I used to have the ics provider hooked up to the request classes as well. I can do some digging, but I think it shouldn't be too much work to adapt.

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: bug-1546606-refactor-caldav-request-handling.diff

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9060339 [details] [diff] [review]
bug-1546606-refactor-caldav-request-handling.diff

Review of attachment 9060339 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/providers/caldav/modules/calDavRequest.jsm
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +ChromeUtils.import("resource://calendar/modules/calUtils.jsm");

things like these need to use the new format of course.

::: calendar/providers/caldav/modules/calDavUtils.jsm
@@ +9,5 @@
> +/* exported xmlns, tagsToXmlns, caldavNSUnresolver, caldavNSResolver, caldavXPath,
> + *          caldavXPathFirst */
> +this.EXPORTED_SYMBOLS = [
> +    "xmlns", "tagsToXmlns", "caldavNSUnresolver", "caldavNSResolver", "caldavXPath",
> +    "caldavXPathFirst"

I think it would be preferable if we start to move more towards one module one export (in general). Then it's much clearer where the global functions come from when you access it using something like CalDavUtils.caldavNSResolver - which could then also just be named CalDavUtils.nsResolver.

Oh, and module files with Capital naming.

Comment 3

[Magnus Melin [:mkmelin]]

Sending over to Paul's backlog.

Comment 4

[Paul Morris [:pmorris]]

Attachment: wip-part1-rebase-original-patch-0.patch

WIP 1 of 5. Just uploading some work in progress. The overview is I've rebased Philipp's original patch on current trunk and then made some changes, most per Magnus' comments. There's more to do, but it's not clear to me yet exactly how much. For example the xpcshell test fails at line 495. Known todos are tagged with "TODO DAV" comments.

Comment 5

[Paul Morris [:pmorris]]

Attachment: wip-part2-initial-rework-0.patch

WIP 2 of 5.

Comment 6

[Paul Morris [:pmorris]]

Attachment: wip-part3-single-export-CalDavRequest-0.patch

WIP 3 of 5.

Comment 7

[Paul Morris [:pmorris]]

Attachment: wip-part4-single-export-CalDavUtils-0.patch

WIP 4 of 5.

Comment 8

[Paul Morris [:pmorris]]

Attachment: wip-part5-capitalize-jsm-module-files-0.patch

WIP 5 of 5.

Comment 9

[Paul Morris [:pmorris]]

Attachment: bug-1546606-refactor-caldav-request-handling-2.diff

Philipp's patch re-rebased, with Prettier formatting.

Comment 10

[Paul Morris [:pmorris]]

Attachment: wip-part1-further-work-0.patch

Some more further work on top of Philipp's patch, also rebased with Prettier formatting.

Philipp, when you have a chance I could use some feedback on next steps here. I've got the test running, but it has one failure (noted in comments), and if you comment out the failing part, it fails in two other places. I didn't get far trying to troubleshoot those failures.

I've added "TODO DAV" comments in a few places to mark areas that seem WIP, so you might want to take a look at those. (Some are just reminders to delete something if it ends up not being used.)

This is a summary of some of the changes:

• Use a single export for CalDavRequest
• Use a single export for CalDavUtils
• Capitalize names for the new jsm modules
• Fix the header iterator in test_caldav_requests.js
• Other small test fixes
• Do a basic checkRedirect function implementation, etc.
• Various edits and updates to satisfy eslint, etc.

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9091923 [details] [diff] [review]
wip-part1-further-work-0.patch

Review of attachment 9091923 [details] [diff] [review]:
-----------------------------------------------------------------

Hope these comments are helpful in finding those errors. Ping me if you'd like me to apply and test.

::: calendar/providers/caldav/calDavCalendar.js
@@ +1423,5 @@
> +  },
> +
> +  /**
> +   * TODO DAV: This function is now never called. It used to be the first in the chain of async
> +   * functions. No longer needed?

Yeah if this isn't used anymore I think it can go. It was originally the function that set up the OAuth keys.

::: calendar/providers/caldav/calDavRequestHandlers.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* globals XML_HEADER MIME_TEXT_XML */

I'm wondering if we can move this file to a JSM and maybe just re-define these consts?

::: calendar/providers/caldav/modules/CalDavRequest.jsm
@@ +8,4 @@
>  
> +var { CalDavSession } = ChromeUtils.import("resource://calendar/modules/caldav/CalDavSession.jsm");
> +
> +this.EXPORTED_SYMBOLS = ["CalDavRequest"]; /* exported CalDavRequest */

I think it would be better to keep exporting the single symbols. This keeps the constructors short, and when we move to es6 imports I'm thinking it would be better to use this:

import { ItemRequest } from "resource://calendar/modules/caldav/CalDavRequest.jsm";

It also makes the no-undef rule more useful, otherwise eslint won't know which properties are defined and which are not. If in an import you'd really want everything you could still use import * as CalDavRequest from ...

@@ +16,5 @@
>  
>  /**
>   * Base class for a caldav request.
>   */
> +class CalDavRequestBase {

Do we use this class on its own? If so I think we should drop the `Base` part.

@@ +474,5 @@
>            if (aProp == "OnStartRequest") {
>              return function(...args) {
>                try {
>                  let result = aTarget[aProp].apply(this, args);
> +                this._onresponded();

Does this work? We are returning a traditional function here, so `this` should be whatever that is called with, which I believe is the object that is being proxied, so `this.request._handler` from line 472?

::: calendar/providers/caldav/modules/CalDavSession.jsm
@@ +116,5 @@
>      });
>    }
>  
>    /**
> +  /**

Double opening comment?

::: calendar/providers/caldav/modules/CalDavUtils.jsm
@@ +7,5 @@
>  /**
>   * Various utility functions for the caldav provider
>   */
>  
> +this.EXPORTED_SYMBOLS = ["CalDavUtils"]; /* exported CalDavUtils */

Same comment about possibly keeping exports

::: calendar/test/unit/test_caldav_requests.js
@@ +6,5 @@
>  
> +var { HttpServer } = ChromeUtils.import("resource://testing-common/httpd.js");
> +var { MockRegistrar } = ChromeUtils.import("resource://testing-common/MockRegistrar.jsm");
> +
> +// TODO DAV: LegacySAXRequest is not imported or tested.

I think one of the reasons I didn't test this was that I'd like to get rid of the SAX handler. See also bug 1514666.

@@ +117,5 @@
> +      let headerIterator = function*(enumerator) {
> +        while (enumerator.hasMoreElements()) {
> +          yield enumerator.getNext().QueryInterface(Ci.nsISupportsString);
> +        }
> +      };

What happened to IterSimpleEnumerator?

@@ +894,5 @@
> +  // Errors out on: response.firstRecipient
> +  // See also around line 370 above where the 20180103 date comes from.
> +  //
> +  // ERROR Unexpected exception NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS:
> +  // [JavaScript Error: "invalid date-time value: "2018-01-03T::""

I've seen this error a few times, it was happening when passing a date to a method that expects a date-time or vice versa. Possibly we need to make the wrapper call the right method, or you need to find out why a date is being passed to a function that expects a date-time.

It might just be a matter of making sure that you use DTSTART;VALUE=DATE:20180103 instead of DTSTART:20180103. Libical would gracefull assume the right type.

Comment 12

[Paul Morris [:pmorris]]

Attachment: wip-part1-further-work-1.patch

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #11)

Hope these comments are helpful in finding those errors. Ping me if you'd
like me to apply and test.

Thanks for the helpful feedback.

Yeah if this isn't used anymore I think it can go. It was originally the
function that set up the OAuth keys.

Removed, and some globals along with it.

I'm wondering if we can move this file to a JSM and maybe just re-define
these consts?

Good idea. Flagging it as a TODO for now until we get the test passing.

I think it would be better to keep exporting the single symbols. This keeps
the constructors short, and when we move to es6 imports I'm thinking it
would be better to use this:

import { ItemRequest } from
"resource://calendar/modules/caldav/CalDavRequest.jsm";

It also makes the no-undef rule more useful, otherwise eslint won't know
which properties are defined and which are not. If in an import you'd really
want everything you could still use import * as CalDavRequest from ...

That makes sense to me. I'm fine with changing them back but will hold off for the moment, to let Magnus weigh in again if he wants to.

+class CalDavRequestBase {

Do we use this class on its own? If so I think we should drop the Base part.

It's not used on its own. I added the "Base" to free up "CalDavRequest" to be the single export.

It currently shows up in doc strings but probably shouldn't. I haven't yet found an established JSDoc way to indicate a param is "any subclass of this abstract base class".

Does this work? We are returning a traditional function here, so this
should be whatever that is called with, which I believe is the object that
is being proxied, so this.request._handler from line 472?

Good point, fixed now with "self" capturing the correct "this".

I think one of the reasons I didn't test this was that I'd like to get rid
of the SAX handler. See also bug 1514666.

OK.

What happened to IterSimpleEnumerator?

It went away in m-c, and was replaced in c-c with code like I used here.

I've seen this error a few times, it was happening when passing a date to a
method that expects a date-time or vice versa. Possibly we need to make the
wrapper call the right method, or you need to find out why a date is being
passed to a function that expects a date-time.

It might just be a matter of making sure that you use
DTSTART;VALUE=DATE:20180103 instead of DTSTART:20180103. Libical would
gracefull assume the right type.

I did some digging and was able to fix this error with some tweaks to icaljs. See more with next uploaded patch.

Comment 13

[Paul Morris [:pmorris]]

Attachment: wip-fix-icaljs-xpcshell-test-0.patch

After some digging I was able to fix the "invalid date-time value" error in the xpcshell test by making a few changes to ical.js.

Basically ical.js assumes that a "period" involves date-times not dates, but the free-busy input in the test is a period that uses dates:

    FREEBUSY;FBTYPE=FREE:20180103/20180117
    FREEBUSY;FBTYPE=BUSY:20180118/P7D

This demonstration patch just makes a few changes to prevent the error in the test. More would need to be done to fully support this case, and of course in the actual ical.js repo.

After this error is fixed, two more errors appear, which I haven't really looked into yet. Here's the end of the log:

0:04.87 pid:5532 console.log: Lightning: CalDAV: Status 207 on initial PROPFIND for calendar xpcshell
 0:04.87 pid:5532 console.error: Lightning: 
 0:04.87 pid:5532   [calCachedCalendar] replay action failed: <unknown>, uri=http://localhost:40105/calendars/xpcshell/events, result=2147500037, operation=[xpconnect wrapped calIOperation]
 0:04.87 pid:5532 console.log: Lightning: [calCachedCalendar] replayChangesOn finished.
 0:04.87 pid:5532 console.log: Lightning: [calCachedCalendar] sync queue empty.
 0:04.87 ERROR Unexpected exception 2147500037
undefined
 0:04.87 INFO exiting test
 0:04.89 pid:5532 JavaScript error: resource://testing-common/PromiseTestUtils.jsm, line 112: uncaught exception: Object
 0:04.94 TEST_END: Test FAIL, expected PASS. Subtests passed 121/121. Unexpected 0 - xpcshell return code: 0
 0:04.95 INFO INFO | Result summary:
 0:04.95 INFO INFO | Passed: 0
 0:04.95 INFO INFO | Failed: 2
 0:04.95 INFO INFO | Todo: 0
 0:04.95 INFO INFO | Retried: 0
 0:04.95 SUITE_END

xpcshell
~~~~~~~~
Ran 244 checks (242 subtests, 2 tests)
Expected results: 242
Unexpected results: 2
  test: 2 (2 fail)

Unexpected Results
------------------
xpcshell-icaljs.ini:comm/calendar/test/unit/test_caldav_requests.js
  FAIL xpcshell-icaljs.ini:comm/calendar/test/unit/test_caldav_requests.js - xpcshell return code: 0
xpcshell-libical.ini:comm/calendar/test/unit/test_caldav_requests.js
  FAIL xpcshell-libical.ini:comm/calendar/test/unit/test_caldav_requests.js - xpcshell return code: 0
ERROR Unexpected exception 2147500037
undefined
ERROR Unexpected exception 2147500037
undefined

And if I revert line 347 in calCachedCalendar.js to (operation ? operation.id : "<unknown>") +
(https://searchfox.org/comm-central/source/calendar/base/src/calCachedCalendar.js#347)

Then you get this slight variation:

0:04.55 pid:7953 console.log: Lightning: CalDAV: Status 207 on initial PROPFIND for calendar xpcshell
 0:04.55 pid:7953 JavaScript strict warning: resource://calendar/calendar-js/calCachedCalendar.js, line 347: ReferenceError: reference to undefined property "id"
 0:04.55 pid:7953 console.error: Lightning: 

Comment 14

[Magnus Melin [:mkmelin]]

(In reply to Paul Morris [:pmorris] from comment #12)

I think it would be better to keep exporting the single symbols. This keeps
the constructors short, and when we move to es6 imports I'm thinking it
would be better to use this:

import { ItemRequest } from
"resource://calendar/modules/caldav/CalDavRequest.jsm";

It also makes the no-undef rule more useful, otherwise eslint won't know
which properties are defined and which are not. If in an import you'd really
want everything you could still use import * as CalDavRequest from ...

That makes sense to me. I'm fine with changing them back but will hold off for the moment, to let Magnus weigh in again if he wants to.

Agreed. Personally, I would rename some/most of the classes to be prefixed the names with CalDav though to make it more clear what they are/do. Like CalDavPropfindRequest.

Comment 15

[Paul Morris [:pmorris]]

Attachment: wip-part1-further-work-2.patch

I've moved this back to exporting multiple classes per module and following mkmelin's suggestion I've renamed the classes using a "CalDav" prefix.

Despite my efforts, I haven't had much success debugging the new test errors (ERROR Unexpected exception 2147500037 undefined). They seem fairly obscure. Philipp, let me know if you have any ideas on that.

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

2147500037 is unfortunately NS_ERROR_FAILURE so not very helpful. I'd suggest using add_task(...).only() to determine the exact test that is failing and then try to debug from there. You can also try using --jsdebugger.

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Running test_freebusy_request I happened to see this, despite the test passing. We should make sure the test fails in this case and we fix the recursion error.

 0:05.17 INFO (xpcshell/head.js) | test test_freebusy_request finished (2)
 0:05.18 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "too much recursion" {file: "resource://calendar/modules/ical.js" line: 5764}]"
 0:05.18 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "too much recursion" {file: "resource://calendar/modules/ical.js" line: 5727}]"

Looking at the last test, which fails reliably for me, I added a try/catch with console.error, this explains the 2147500037 error a bit better:

 0:01.63 pid:11712 console.error: Lightning:
 0:01.63 pid:11712   [calCachedCalendar] replay action failed: <unknown>, uri=http://localhost:55773/calendars/xpcshell/events, result=2147500037, operation=[xpconnect wrapped calIOperation]
 0:01.63 pid:11712 console.log: Lightning: [calCachedCalendar] replayChangesOn finished.
 0:01.63 pid:11712 console.log: Lightning: [calCachedCalendar] sync queue empty.
 0:01.63 pid:11712 console.error: 2147500037

I attached the debugger and stepped through, and the issue is within the checkRedirect() function. You have a TODO to implement that function, and I think if you do then the tests should work better.

The function name might be a bit misleading. What the function should do is return true if the request was not redirected (which would be your quick fix for the tests). If it was redirected, prompt the user if they want to accept following that redirect, if they cancel then return false to disable the calendar. If they do accept the redirect, you may also need to see if the calendar URI needs to be adapted.

Doing this we get a bit further. I did notice this warning which looks like an underlying error, but possibly not related to the test. Definitely something to investigate:

 0:01.91 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "QueryInterface"" {file: "file:///Users/kewisch/mozilla/gecko/objdir-tb-artifact/dist/Thunderbird%20Daily.app/Contents/Resources/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calStorageCalendar.js" line: 2331}]"

Anyway, the caldav calendar doesn't seem to be returning the item that should be stored in the equal(items.length, 1); check, I'll leave this to you to figure out :)

As for your question if you can use CalDavResponseBase in the docstring, I'd say just do it. I don't think eslint checks for exported symbols, but if the eslint jsdoc plugin complains that is something to look into.

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

Note also I had to add the debugger keyword to the test to make attaching the js debugger work well. There seem to be a few things in devtools where Thunderbird might need some fixes down the line, but with that workaround you can at least debug.

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9092688 [details] [diff] [review]
wip-fix-icaljs-xpcshell-test-0.patch

Review of attachment 9092688 [details] [diff] [review]:
-----------------------------------------------------------------

So you are experiencing a PERIOD with a DATE as opposed to a DATE-TIME? As per https://tools.ietf.org/html/rfc5545#section-3.3.9 a PERIOD should only accept a DATE-TIME, anything else is wrong. Does libical accept a DATE? I'd prefer if ical.js could stick to the spec. I was considering a quirks mode for ical.js but that would require a deeper architectural change. Given I won't have time to do that soon I guess accepting DATE would be ok, but maybe add a comment that this is not per spec.

Comment 20

[Paul Morris [:pmorris]]

Attachment: part2-wip-fixing-tests-caldav-refactor-0.patch

Thanks again for the feedback. Getting back to this after working on other things for awhile. This patch continues fixing test related errors and failures. I've also rebased the other patches locally, will upload new versions of those.

test_caldav_client
I stubbed out the checkRedirect function correctly to solve that issue. Then using the debugger (which required using --flag icaljs or --flag libical to run only a single test), I looked into the next failure with getAllItems returning an empty array. With the debugger running the test did not fail, and I could see that the async call to the database in getItems (here: https://searchfox.org/comm-central/source/calendar/providers/storage/calStorageCalendar.js#737) was returning a result, and the callback function was called. Without the debugger that async call to the database was resolving but without returning a result, and the callback wasn't getting called. It appears that the test was checking for something that is not there yet. Also, if I made two calls to getAllItems in the test, the first would return an empty array and the second would return the expected result. So adding executeSoon got the test to pass. (And also eliminated that warning about QueryInterface being undefined that was apparently related.)

test_freebusy_request
Given what you wrote on this, I took a new approach and changed the input to be correct -- DATE-TIME rather than just DATE. (Maybe we want to test incorrect input too, eventually but first things first.) That solved the problem where ical.js was expecting a DATE-TIME but was getting a DATE. However, ical.js and libical produce different results for the last deepEqual in the test given the same input, so one or the other will fail. Do we want to tweak ical.js output to match libical? Or...?

test_freebusy_request - too much recursion issue
I looked into this briefly, but haven't gotten to the bottom of it. Will keep working on it (and the other TODOs that aren't test related).

Comment 21

[Paul Morris [:pmorris]]

Attachment: bug-1546606-refactor-caldav-request-handling-3.diff

Rebased on current trunk.

Comment 22

[Paul Morris [:pmorris]]

Attachment: wip-part1-further-work-3.patch

Rebased. I can squash these patches eventually, but keeping them separate for now for easier review/feedback.

Comment 23

[Paul Morris [:pmorris]]

The "too much recursion" error in test_freebusy_request happens when this assertion fails:
strictEqual(first, response.data["mailto:[e-mail]"]);
which causes JSON.stringify to be called on the arguments to that assertion (to log the error), and that causes the runaway recursion. You can also hit it by just calling JSON.stringify(first). Here's the trace of that (which is similar to the trace with strictEqual).

 0:09.82 pid:11878 resource://calendar/modules/ical.js 4950 fromData
 0:09.82 pid:11878 resource://calendar/modules/ical.js 4813 icaltime
 0:09.82 pid:11878 resource://calendar/modules/ical.js 4866 clone
 0:09.82 pid:11878 resource://calendar/modules/ical.js 5044 startOfWeek
 0:09.82 pid:11878 resource://calendar/calendar-js/calDateTime.js 165 get startOfWeek
 0:09.82 pid:11878 [...]/xpcshell/comm/calendar/test/unit/test_caldav_requests.js 899 test_freebusy_request

I don't yet know where the call to startOfWeek comes from. (The JS debugger connects but execution doesn't stop for debugging, even though I'm using the "debugger;" statement.) Maybe it's something with the timezone argument in the call to fromData , which is {wrappedJSObject:{}, expandedUntilYear:0, changes:[], tzid:"floating"}.

Comment 24

[Magnus Melin [:mkmelin]]

For figuring out where calls come from, often putting a console.trace() at the interesting place will help a bunch.

Comment 25

[Paul Morris [:pmorris]]

(In reply to Magnus Melin [:mkmelin] from comment #24)

For figuring out where calls come from, often putting a console.trace() at the interesting place will help a bunch.

Thanks, yeah, I've been using console.trace when I can't get the debugger working. It's how I got the log above. But if I put a console.trace() in the startOfWeek call I don't get any more info about how we get from JSON.stringify(first) in the test to the startOfWeek call:

console.trace: 
 0:12.96 pid:24864 resource://calendar/calendar-js/calDateTime.js 167 get startOfWeek
 0:12.96 pid:24864 [...]/xpcshell/comm/calendar/test/unit/test_caldav_requests.js 903 test_freebusy_request
 0:12.96 pid:24864 [...]/xpcshell/head.js 1567 run_next_test/_run_next_test/<
 0:12.96 pid:24864 [...]/xpcshell/head.js 1567 _run_next_test
 0:12.96 pid:24864 [...]/xpcshell/head.js 735 run
 0:12.96 pid:24864 [...]/xpcshell/head.js 246 _do_main
 0:12.96 pid:24864 [...]/xpcshell/head.js 573 _execute_test

I've spent enough time on this test that I think it may make sense to defer fixing it to a follow-up bug. Maybe just comment out this strictEqual assert for now. [EDIT: no need to comment it out, see next comment.] It succeeds in libical, so we have an icaljs issue and not a problem with the caldav refactor changes.

Comment 26

[Paul Morris [:pmorris]]

After backing out my WIP debugging commit and looking again, I see that the strictEqual(first, response.data["mailto:[e-mail]"]); assertion succeeds with ical.js. Apparently when logging a success or a failure, Assert.jsm tries to do JSON.stringify and if that fails it falls back to toString. The "too much recursion" happens in the JSON.stringify attempt but the assert still succeeds.

0:20.39 PASS test_freebusy_request - [test_freebusy_request : 898] [object Object] === [object Object]

I'll open a new bug for the recursion error and add a comment to the test about it, but keep the assertion.

Comment 27

[Paul Morris [:pmorris]]

Attachment: part2-wip-fixing-tests-caldav-refactor-1.patch

I filed https://github.com/mozilla-comm/ical.js/issues/410 for the "too much recursion" issue, and made a comment in the test with this patch.

This patch also fixes the test_freebusy_request error where ical.js was expecting a DATE-TIME but was getting a DATE (see comment 20 above). By changing the FREEBUSY part of the response to a DATE-TIME and leaving the DTSTART and DTEND parts untouched... ical.js and libical produce the same results for this assertion, so it passes for both:

deepEqual(
    first.intervals.map(interval => interval.begin.icalString + ":" + interval.end.icalString),
    [
      "20180101:20180102",
      "20180103T010101Z:20180117T010101Z",
      "20180118T010101Z:20180125T010101Z",
      "20180126:20180201",
    ]
  );

With this patch all the new tests pass. Next is implementing the checkRedirect dialog, moving calDavRequestHandlers.js to a jsm, and the raiseForStatus error handling in CalDavRequest.jsm.

Comment 28

[Paul Morris [:pmorris]]

Attachment: part3-wip-move-request-handlers-to-jsm-0.patch

Moves calDavRequestHandlers.js to a .jsm.

Comment 29

[Paul Morris [:pmorris]]

Looking at this redirect dialog again, I realize I'm not sure about some of the details. Philipp you wrote:

If the URI was redirected, prompt the user if they want to accept following that redirect, if they cancel then return false to disable the calendar. If they do accept the redirect, you may also need to see if the calendar URI needs to be adapted.

Can you explain what you mean by "see if the calendar URI needs to be adapted"? Do you mean let the user click a button to update the calendar's URI to be the target URI of the redirect? Or do we just do that automatically on accepting a redirect? But only for some types of redirects and not others?

My sense is that the dialog should show the user the current URI and the URI where it is being redirected. There should be "cancel" and "accept redirect" buttons that either disables the calendar or accepts the redirect. On accept also do something about adapting the URI.

(cc'ing aleca since this will be a UI addition. This is a dialog that will appear when the URI of a remote CalDav calendar is being redirected.)

Comment 30

[Paul Morris [:pmorris]]

Attachment: part4-wip-add-check-redirect-dialog-0.patch

Adds a basic dialog to prompt the user when a calendar URI is being redirected.

Comment 31

[Paul Morris [:pmorris]]

Attachment: Calendar-URI-Redirect-Dialog-0.png

Screenshot of a first pass at this URI redirect dialog.

Comment 32

[Paul Morris [:pmorris]]

Attachment: part5-wip-response-errors-0.patch

This defines the missing classes for the various response errors.

The raiseForStatus function where these errors are thrown is still not called anywhere. These types of errors are handled in calDavCalendar.js (e.g. with response.serverError, response.clientError, response.authError, etc.). Should raiseForStatus be called there in the various places where these errors are handled?

There's a response.nsirequest property that's used in various places (like the getter for response.status), but I don't see it defined or set on the CalDavResponseBase class or its sub-classes. Is that one of the things that's missing or am I missing something?

Of the TODOs I identified, there are only two left: this error stuff and the redirect dialog above. I'll need some feedback in order to take this further.

Comment 33

[Magnus Melin [:mkmelin]]

Comment on attachment 9113821 [details] [diff] [review]
part2-wip-fixing-tests-caldav-refactor-1.patch

Review of attachment 9113821 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/providers/caldav/calDavCalendar.js
@@ +1410,5 @@
>     * if they want to adapt the URL.
>     *
>     * TODO DAV: this function was not implemented yet. This is just a basic implementation to get
>     * the test to run.
> +   * The function name might be a bit misleading.

indeeed

@@ +1419,3 @@
>     *
> +   * @param {CalDavResponseBase} aResponse - Response to check (a subclass of CalDavResponseBase).
> +   * @return {boolean}                     - True if the request was not redirected or if the

nit: for documentation, I think indention like this doesn't scale (you add one param, and then everything is off). 
I'd do it like this: (and for return, no -)
@return {boolean}  true if the request was not redirected

If it wraps, the next line start would align with e under @return

Comment 34

[Paul Morris [:pmorris]]

(In reply to Magnus Melin [:mkmelin] from comment #33)

nit: for documentation, I think indention like this doesn't scale (you add
one param, and then everything is off).
I'd do it like this: (and for return, no -)
@return {boolean} true if the request was not redirected

If it wraps, the next line start would align with e under @return

I definitely agree on avoiding the style where the wrapping doesn't scale. In that regard I like this style found in some m-c files, which I find really readable, and would be simple to write consistently: Edit: here's the link: https://searchfox.org/mozilla-central/source/dom/tests/browser/browser_hasbeforeunload.js#12

I like using "-" for return, if/since we're using it for param, for greater consistency. It's simpler if you always use it, but I don't feel that strongly about it.

I see you put two spaces between {boolean} true. We should really document all these conventions and expectations somewhere, especially as they get tighter/more specific than the JSDoc docs, which are pretty loose. If only Prettier could auto-format JSDoc strings. :)

Comment 35

[Philipp Kewisch [:Fallen][📱📆]]

Personally I prefer the version that doesn't scale and just reindent if necessary. Blame for doc params isn't that interesting. What I have been doing in calendar normally is tabbing to the next tabstop after the longest param and then aligning the others. I don't particularly like the compact version because I feel it decreases readability, which is what docs are all about.

Comment 36

[Magnus Melin [:mkmelin]]

(In reply to Paul Morris [:pmorris] from comment #34)

I definitely agree on avoiding the style where the wrapping doesn't scale. In that regard I like this style found in some m-c files, which I find really readable, and would be simple to write consistently: Edit: here's the link: https://searchfox.org/mozilla-central/source/dom/tests/browser/browser_hasbeforeunload.js#12

That looks good too, but still cause inconsistency for the common case that you only need the one line.

I like using "-" for return, if/since we're using it for param

Well, JSDoc examples never use hypen for return.

I see you put two spaces between {boolean} true.

Typo ;) Just one space please.

Comment 37

[Philipp Kewisch [:Fallen][📱📆]]

For the record, I added a lenient mode to upstream ical.js that parses the dates better. Also a lot of other PRs are merged. We might want to spin a release and then pull from upstream. Paul, just checking in on this bug, I'm a bit lost what feedback you need from me and I don't want to block you on this. Anything I can help with?

(In reply to Paul Morris [:pmorris] from comment #29)

Looking at this redirect dialog again, I realize I'm not sure about some of the details. Philipp you wrote:

If the URI was redirected, prompt the user if they want to accept following that redirect, if they cancel then return false to disable the calendar. If they do accept the redirect, you may also need to see if the calendar URI needs to be adapted.

Can you explain what you mean by "see if the calendar URI needs to be adapted"? Do you mean let the user click a button to update the calendar's URI to be the target URI of the redirect? Or do we just do that automatically on accepting a redirect? But only for some types of redirects and not others?

The previous dialog always prompted the user in case of a redirect if they want to accept the target of the redirect as the new calendar uri. Just to avoid a misconfigured webserver changing the calendar uri.

My sense is that the dialog should show the user the current URI and the URI where it is being redirected. There should be "cancel" and "accept redirect" buttons that either disables the calendar or accepts the redirect. On accept also do something about adapting the URI.

(cc'ing aleca since this will be a UI addition. This is a dialog that will appear when the URI of a remote CalDav calendar is being redirected.)

I'm fine with changing how the dialog looks, maybe in a followup though?

Comment 38

[Paul Morris [:pmorris]]

Thanks Philipp. Sounds good on ical.js. This bug has been on the back burner while I've been focusing on calendar integration. From your comments it sounds like I'm on the right track with the dialog and what it should do. I think my question was basically: does clicking "accept redirect" in the dialog mean:

(a) Make the target URI of the redirect the new URI for the calendar, forget the old one, and use the new one going forward.

(b) Accept just a one-time redirect, the user will see the dialog again the next time the calendar data is synced, and also show a second dialog asking if the user wants to accept the new URI "permanently" as the new URI for the calendar, as a separate question.

I think (a) is better, because simpler, and seems to be what you have in mind. Let me know if that's not the case.

(I think I was uncertain about what you meant with the phrase "If they do accept the redirect, you may also need to see if the calendar URI needs to be adapted". Namely, the "may" which I read as you "may or may not" need to do anything more than just a one-time redirect, and then the "also need to see if", did that mean present a second dialog to ask the user if ("see if") the calendar URI should be updated to the target of the redirect. I think I've got it straight now, but that's where I got confused. Thanks again for clarifying.)

The previous dialog always prompted the user

Huh, there was a previous dialog? I hadn't seen one, must be missing it somehow.

Comment 39

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Paul Morris [:pmorris] from comment #38)

I think my question was basically: does clicking "accept redirect" in the dialog mean:

(a) Make the target URI of the redirect the new URI for the calendar, forget the old one, and use the new one going forward.

(b) Accept just a one-time redirect, the user will see the dialog again the next time the calendar data is synced, and also show a second dialog asking if the user wants to accept the new URI "permanently" as the new URI for the calendar, as a separate question.

Yes, I meant (a).

The previous dialog always prompted the user

Huh, there was a previous dialog? I hadn't seen one, must be missing it somehow.

The previous one is fairly simple, it is triggered here: https://searchfox.org/comm-central/source/calendar/providers/caldav/calDavCalendar.js#1821

Comment 40

[Paul Morris [:pmorris]]

Attachment: part0-bug-1546606-refactor-caldav-request-handling-4.diff

0 of 5. This is Philipp's original patch; I've just been rebasing it. I've flagged darktrojan for review since it's a pretty significant change.

In the next patch there's follow-up work I've done to use current idioms and conventions, so no need to review for those things here if they are covered in that patch. Apologies if that's a pain. I usually prefer working with smaller, separate patches; happy to merge them if that makes more sense.

Comment 41

[Paul Morris [:pmorris]]

Attachment: part1-refactor-caldav-request-handling-some-more-4.patch

1 of 5. Follow-up to part0. Here's most of what's going on here, from the commit message:

• Capitalize names for the new jsm modules
• Prefix exported classes with CalDav
• Fix the header iterator in test_caldav_requests.js
• Other small test fixes
• Do a basic checkRedirected function implementation, etc.
• Various edits and updates to satisfy eslint, etc.

Comment 42

[Paul Morris [:pmorris]]

Attachment: part2-fix-tests-caldav-2.patch

2 of 5. Fixes the new unit tests that were failing.

Comment 43

[Paul Morris [:pmorris]]

Attachment: part3-move-request-handlers-to-jsm-1.patch

3 of 5. Converts the request handlers JS file to a JSM.

Comment 44

[Paul Morris [:pmorris]]

Attachment: part4-add-uri-redirect-dialog-1.patch

4 of 5. Adds a URI redirect dialog. Now using an xhtml instead of a xul file.

The previous one is fairly simple, it is triggered here: https://searchfox.org/comm-central/source/calendar/providers/caldav/calDavCalendar.js#1821

Now that I see this simpler way of implementing the dialog, it looks like the better way to do it. It looks like that method supports everything that I'm doing with this new dialog but with fewer moving parts.

Philipp, if you agree I'll just take some feedback and rework this to use that simpler method, otherwise feel free to review what I've done in this patch. :)

Comment 45

[Paul Morris [:pmorris]]

Attachment: Calendar-URI-Redirect-Dialog-1.png

Screenshot of current state of the dialog from part4 patch in previous comment.

Comment 46

[Paul Morris [:pmorris]]

Attachment: part5-add-response-error-classes-1.patch

5 of 5. Adds error classes needed by the raiseForStatus function. This function is currently not called anywhere, but better to add the classes it needs so it's ready to go and avoid no-undef eslint exceptions.

With these patches applied I tested by subscribing to a read-only fastmail calendar via caldav. The unit tests pass locally, and I'll add a try run to this comment shortly.

Edit: try run just started: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=68161785182a29648fb8aa74660d95cae648a6c8

Comment 47

[Paul Morris [:pmorris]]

Looks like I should have done a try run before uploading the patches. I've now fixed the issues locally:

• eslint issue: converted to using method shorthand in three places
• build issue: removed the now non-existent calDavRequestHandlers.js from package-manifest.in

Here's another try run showing it's working now with those changes.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2f3cf7b822b02c1823e5b7cb3dff545496cbd2b2

I'll hold off on uploading the latest version of the patches for now since the changes are minor.

Comment 48

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9129182 [details] [diff] [review]
part1-refactor-caldav-request-handling-some-more-4.patch

Review of attachment 9129182 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/src/calCachedCalendar.js
@@ +344,5 @@
>              let status = operation ? operation.status : Cr.NS_OK;
>              if (!Components.isSuccessCode(status)) {
>                cal.ERROR(
>                  "[calCachedCalendar] replay action failed: " +
> +                  (operation && operation.id ? operation.id : "<unknown>") +

New trick I learned if you like it, works since Gecko 74:

operation?.id || "<unknown>"

Comment 49

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9129183 [details] [diff] [review]
part2-fix-tests-caldav-2.patch

Review of attachment 9129183 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/test/unit/test_caldav_requests.js
@@ +913,5 @@
>  add_task(async function test_caldav_client() {
>    let client = await gServer.getClient();
>    let pclient = cal.async.promisifyCalendar(client);
>  
> +  executeSoon(async () => {

I think this will cause the inner function to execute after the task completes, wouldn't you need to await this? Not sure why we need to executeSoon here though, if there is a race condition let's see if we can fix it instead.

Comment 50

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9129188 [details] [diff] [review]
part4-add-uri-redirect-dialog-1.patch

Review of attachment 9129188 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/base/content/dialogs/calendar-uri-redirect-dialog.xhtml
@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> +
> +<!DOCTYPE window SYSTEM "chrome://calendar/locale/calendar-uri-redirect-dialog.dtd">

I wonder if we could use fluent here instead? This bug is long enough though, I can understand if you just want to get it in.

@@ +12,5 @@
> +        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +        xmlns:html="http://www.w3.org/1999/xhtml"
> +        title="&calendarURIRedirect.title;">
> +
> +  <dialog id="calendar-uri-redirect-dialog"

I guess you can't easily due to system styling of the dialog element, but this seems like a dialog that would be great to write in pure html without xul.

::: calendar/providers/caldav/CalDavCalendar.jsm
@@ +1426,5 @@
> +
> +    if (args.returnValue) {
> +      this.uri = response.uri;
> +      // TODO: Is this line needed?  It was used in the previous dialog.
> +      // this.setProperty("uri", response.uri.spec);

This is what sets the calendar URI to the redirected URI, are you doing this somewhere else?

Comment 51

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9129181 [details] [diff] [review]
part0-bug-1546606-refactor-caldav-request-handling-4.diff

Review of attachment 9129181 [details] [diff] [review]:
-----------------------------------------------------------------

::: calendar/providers/caldav/CalDavCalendar.jsm
@@ +1593,5 @@
> +          });
> +          cal.LOG(
> +            `Adding supported items: ${this.mSupportedItemTypes.join(",")} for calendar: ${
> +              this.name
> +            }`

Ha, prettier can make things so...pretty...sometimes.

::: calendar/providers/caldav/modules/calDavRequest.jsm
@@ +30,5 @@
> + */
> +class CalDavRequest {
> +  QueryInterface(aIID) {
> +    return cal.generateClassQI(this, aIID, [Ci.nsIChannelEventSink, Ci.nsIInterfaceRequestor]);
> +  }

Another new thing you could do, public class fields are supported since Firefox 69 now: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Class_fields#Public_fields

class CalDavRequest {
  QueryInterface = ChromeUtils.generateQI([Ci.nsIChannelEventSink, Ci.nsIInterfaceRequestor])
  ...
}

Maybe it can even be made static.

(note this version doesn't use cal.generateQI, which may make it necessary to explicitly QI because nsIClassInfo is not filled out. Ping me on irc if you need more details about this)

Comment 52

[Geoff Lankow (:darktrojan)]

Apologies for dragging the chain on this, I was actually half-way through a review but I've not had a chance to finish it.

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #51)

public class fields are supported since Firefox 69 now:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Class_fields#Public_fields

I didn't know that! I'd gotten so used to them not being supported.

Comment 53

[Paul Morris [:pmorris]]

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #48)

New trick I learned if you like it, works since Gecko 74:

operation?.id || "<unknown>"

Oh nice, cool that that's in Gecko now. I tried it but unfortunately eslint doesn't support it yet, and it doesn't seem worth adding an eslint exception for that line. https://github.com/eslint/eslint/issues/12642

Comment 54

[Paul Morris [:pmorris]]

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #51)

Another new thing you could do, public class fields are supported since Firefox 69 now:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/
Class_fields#Public_fields

Good to know. Unfortunately, it looks like eslint is not supporting this yet either (still stage 3).

So I think the outstanding things for me to address here are:

• Convert the dialog to fluent.
• The race issue that comes up in the tests.

Comment 55

[Paul Morris [:pmorris]]

Attachment: part4-add-uri-redirect-dialog-2.patch

Now using Fluent. I couldn't get the custom button labels to work with Fluent, but the standard "Cancel"/"OK" is fine here, and arguably better, so let's just do that.

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #50)

I guess you can't easily due to system styling of the dialog element, but
this seems like a dialog that would be great to write in pure html without
xul.

Yeah, good point. Leaving it as xul for now, should be easy to convert when we convert dialogs to html.

::: calendar/providers/caldav/CalDavCalendar.jsm
@@ +1426,5 @@

if (args.returnValue) {
  this.uri = response.uri;
  // TODO: Is this line needed?  It was used in the previous dialog.
  // this.setProperty("uri", response.uri.spec);

This is what sets the calendar URI to the redirected URI, are you doing this
somewhere else?

Ah, okay, I was thinking the line above covered that (this.uri = response.uri). I should look into the difference between those two ways of setting a property. I've put both lines in in this patch.

Comment 56

[Paul Morris [:pmorris]]

Attachment: part2-fix-tests-caldav-3.patch

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #49)

executeSoon(async () => {

I think this will cause the inner function to execute after the task
completes, wouldn't you need to await this? Not sure why we need to
executeSoon here though, if there is a race condition let's see if we can
fix it instead.

In the interest of landing these patches, I propose fixing the race condition in a follow-up bug. Below is the diff between this patch and the previous version. The change solves the need to await so the task doesn't complete before we're actually done.

Geoff, Philipp has r+'d the previous version of the patch. Can you review the change below? I'll open a follow-up bug to look further into this after these patches land.

-  executeSoon(async () => {
-    let items = await pclient.getAllItems();
-    equal(items.length, 1);
-  });
+  // TODO: Fix the problem that is causing this test to fail when the next line is removed.
+  await new Promise(resolve => executeSoon(resolve));
+
+  let items = await pclient.getAllItems();
+  equal(items.length, 1);

Comment 57

[Paul Morris [:pmorris]]

Comment on attachment 9132399 [details] [diff] [review]
part4-add-uri-redirect-dialog-2.patch

Geoff, if you have a chance could you review this for how I'm using Fluent for the dialog?  Philipp has r+'d the previous version of the patch and the only difference is using Fluent for the strings.

Comment 58

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9132682 [details] [diff] [review]
part2-fix-tests-caldav-3.patch

Review of attachment 9132682 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe I am missing something, but I think this might not be doing what you want it to?

::: calendar/providers/caldav/CalDavCalendar.jsm
@@ +1413,1 @@
>      return response.redirected;

Isn't this mostly equivalent to `return true;`?  If response.redirected is falsy, return true. Otherwise return a truthy value.

Comment 59

[Paul Morris [:pmorris]]

Attachment: part1-refactor-caldav-request-handling-some-more-5.patch

Fixes the eslint issue from the try run (introduced by new version of Prettier).

Comment 60

[Paul Morris [:pmorris]]

Attachment: part2-fix-tests-caldav-4.patch

Rebased and addressing the following:

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #58)

::: calendar/providers/caldav/CalDavCalendar.jsm
@@ +1413,1 @@

 return response.redirected;

Isn't this mostly equivalent to return true;? If response.redirected is
falsy, return true. Otherwise return a truthy value.

You are right. This was just a temporary thing to help with getting the tests passing. I've changed it to just return true and added a comment. It is implemented for real in the later patch that adds the URI redirect dialog.

Comment 61

[Paul Morris [:pmorris]]

Attachment: part3-move-request-handlers-to-jsm-2.patch

Just rebased.

Comment 62

[Paul Morris [:pmorris]]

Attachment: part4-add-uri-redirect-dialog-3.patch

Just rebased.

Comment 63

[Paul Morris [:pmorris]]

Attachment: part5-add-response-error-classes-2.patch

Just rebased.

Comment 64

[Paul Morris [:pmorris]]

Attachment: part0-bug-1546606-refactor-caldav-request-handling-5.diff

Final rebases before landing.

Comment 65

[Paul Morris [:pmorris]]

Attachment: part1-refactor-caldav-request-handling-some-more-6.patch

Rebased.

Comment 66

[Paul Morris [:pmorris]]

Attachment: part2-fix-tests-caldav-5.patch

Rebased.

Comment 67

[Paul Morris [:pmorris]]

Attachment: part3-move-request-handlers-to-jsm-3.patch

Rebased.

Comment 68

[Paul Morris [:pmorris]]

Attachment: part4-add-uri-redirect-dialog-4.patch

Rebased.

Comment 69

[Paul Morris [:pmorris]]

Attachment: part5-add-response-error-classes-3.patch

Rebased.

Comment 70

[Paul Morris [:pmorris]]

One more try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=24b812b1ce3c47561a2d3487ef91f703896b99e8

Comment 71

[Paul Morris [:pmorris]]

Attachment: part2-fix-tests-caldav-6.patch

This fixes the linting issue from the try run.

Comment 72

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/eefb833d48f2
Refactor CalDAV request handling. r=pmorris
https://hg.mozilla.org/comm-central/rev/eb5fd4e3c650
Refactor CalDAV request handling some more. r=Fallen
https://hg.mozilla.org/comm-central/rev/9deaa698c34d
Fix the new CalDAV unit tests. r=Fallen
https://hg.mozilla.org/comm-central/rev/b5b3dc5fdf70
Move calDavRequestHandlers.js into a JSM. r=Fallen
https://hg.mozilla.org/comm-central/rev/1295426fff19
Add a CalDAV calendar URI redirect dialog. r=Fallen
https://hg.mozilla.org/comm-central/rev/11272b25eeab
Define classes for the various CalDAV response errors. r=Fallen

Comment 73

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bec9319045eb
Move calDavRequestHandlers.js into a JSM suite part. r=frg