1651783 - Add an "Edit" context menu item to open up the edit dialog directly.

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, task)

Description

[Lasana Murray]

This depends on bug 1575195 being landed. Discussion from there:

For UX, in the context menu when you right-click on an event in a calendar view we should add an "Edit..." item that opens the edit dialog directly

Comment 1

[Lasana Murray]

Attachment: bug1651783.patch

Adds the Edit item to the context menu as well as some test to ensure its disabled when it needs to be.

Comment 2

[Lasana Murray]

Try run in progress:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=359a0dfe3bd1bd5f798bc75356e1667c2177f36b

Comment 3

[Paul Morris [:pmorris]]

Comment on attachment 9177001 [details] [diff] [review]
bug1651783.patch

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

Changes look good, and worked as expected when I tried it.  Great to have test coverage! And using mochitest style.  I have a few suggestions on the tests, mostly mochitest style things I've picked up from Geoff.

::: calendar/test/browser/contextMenu/browser_edit.js
@@ +1,4 @@
> +/* 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/.
> + */

Nit: I think we usually put the `*/` at the end of the previous line, so the license block is just 3 lines.

@@ +16,5 @@
> +let manager;
> +let calendar;
> +let calendarProxy;
> +let menu;
> +let editMenu;

I think it would be better to use local variables inside each add_task test, rather than have these at top level, to make things more self-contained.  Looking at our other browser tests, most don't have top level variables like this, and where they are do it looks like a hold over from mozmill style.  (I'm not 100% sure of best practices here.  Geoff would be a good person to ask about this stuff since he knows mochitests well, and/or look at some of the more recently added mochitests for examples.)

Also there are some helper functions in the calendar/test/browser/head.js file that would come in handy (for opening calendar tab, creating calendars, etc.) (although I think the files may have to be in the same directory, so you might need to move the test file up a level to take advantage of that?).

@@ +18,5 @@
> +let calendarProxy;
> +let menu;
> +let editMenu;
> +
> +async function getEventBox(attrSelector) {

Can you add a brief doc string for this function?

@@ +57,5 @@
> + * Tests the "Edit" menu item is available and opens up the event dialog.
> + */
> +add_task(async function testEditEditableItem() {
> +  let event = new CalEvent();
> +  event.title = "Editable Event";

Since you're checking this in the assertion below, I would do `const title = "..."`, assign it to the `event.title`, then use the const in the assertion so you know you're asserting against a constant.

@@ +75,5 @@
> +    let doc = win.document;
> +    if (doc.documentURI == "chrome://calendar/content/calendar-event-dialog.xhtml") {
> +      let iframe = doc.querySelector("#lightning-item-panel-iframe");
> +      await BrowserTestUtils.waitForEvent(iframe.contentWindow, "load");
> +      return true;

I'd put some assertions in around here to make this easier to debug if the dialog fails to open for some reason.  You might not even need the `if`; instead you could just check that it loaded correctly with an assertion.

@@ +87,5 @@
> +  Assert.ok(
> +    (iframeDoc.querySelector("#item-title").value = event.title),
> +    'context menu "Edit" item opens the editing dialog'
> +  );
> +  editWindow.document.querySelector("dialog").acceptDialog();

This works, and is okay, but I've tended to put all the code that deals with the dialog window in the function passed to `BrowserTestUtils.domWindowOpened`, that way it's all together in one place.  Like, in this case, there's no point in trying to do some of this if the dialog doesn't open correctly.

@@ +140,5 @@
> +    "DTEND:20200103T163000Z\r\n" +
> +    "DESCRIPTION:Just a Test\r\n" +
> +    "SEQUENCE:0\r\n" +
> +    "TRANSP:OPAQUE\r\n" +
> +    "END:VEVENT\r\n";

Oh man, we could really use that `dedent` function sooner than later.  ;-)   (This is fine for now.)

@@ +183,5 @@
> +});
> +
> +registerCleanupFunction(() => manager.removeCalendar(calendar));
> +
> +registerCleanupFunction(() => {

You might as well combine these two cleanup functions into one.

Comment 4

[Lasana Murray]

I think it would be better to use local variables inside each add_task test

So create a new calendar in each test case? I had tried to do that before but it seemed like the clean up functions only run after all the tests run not like say afterEach in a BDD testing style. I moved the menu variables, I'm going to try again with the calendar.

Also there are some helper functions in the calendar/test/browser/head.js file that would come in handy (for opening calendar tab, creating calendars, etc.) (although I think the files may have to be in the same directory, so you might need to move the test file up a level to take advantage of that?).

Those are some of the functions I would like to see in a CalendarTestUtils object. Not sure what the policy is, but I put the test in its own folder in case any future context menu items need to have tests. The tests would have to be moved one level up to benefit from head.js (or at least so it seems).

Comment 5

[Lasana Murray]

Attachment: bug1651783v2.patch

Made most of the changes. Hope it's not too late to request a review. :/

Comment 6

[Lasana Murray]

Another try run just in case:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8d113f25bee2f2fe981dbf15da18ee6bc47e0fd5

Comment 7

[Paul Morris [:pmorris]]

Comment on attachment 9177213 [details] [diff] [review]
bug1651783v2.patch

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

Thanks, looks good overall and ready to land.

As you work on the tests, adding helper functions, etc. I'd ask Geoff what his opinion is on how to handle cases like this, and whether it's better to set up a new calendar for each add_task or whether it makes more sense to set one up for the whole test, and if so, what the best way to do that would be.  Having separate setup steps in each add_task does add a fair bit of boilerplate, so eventually some kind of helper function(s) for this kind of set up would be good, I would think.  

But in the short run, it's great to be adding tests as we add new features.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/49abee5a99cb
Add edit option to calendar item context menu. r=pmorris DONTBUILD