Closed Bug 1530213 Opened 8 months ago Closed 3 months ago

Invite attendees doesn't expand mailing lists anymore

Categories

(Calendar :: E-mail based Scheduling (iTIP/iMIP), defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mkmelin, Assigned: benjamin)

Details

(Keywords: regression)

Attachments

(1 file, 6 obsolete files)

Invite Attendees doesn't expand mailing lists anymore, so it tries to send to the list representation instead, i.e. <testlist>

This broke sometime before start of this year. Forgot to file.

Originally apparently implemented in bug 459021 (for code pointers).

For which version is this reported? Is this an issue in ESR, too?

Flags: needinfo?(mkmelin+mozilla)

This might be bug 758493 - I need to rebase the patch there to check it.

Not an issue on ESR. I'm seeing it on nightlies, IIRC at least since sometime December.

Flags: needinfo?(mkmelin+mozilla)
Version: unspecified → Trunk
Assignee: nobody → benjamin

Looking at it now

As a pointer, resolvePotentialList is likely what you should be checking - or why that doesn't get called.
https://searchfox.org/comm-central/rev/ee05c0d9006626f95dfbb1b6a79bed84e2b90e98/calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js#950

https://searchfox.org/comm-central/rev/ee05c0d9006626f95dfbb1b6a79bed84e2b90e98/calendar/base/content/dialogs/calendar-event-dialog-attendees.js#889

Maybe the event target is not what it used to be, or something.
anonid is (basically) used only in xbl (anonymous content id).

Awesome, I was going to widen my search to things not included in bug 459021 as nothing has changed in ...attendees.js in a while. However, I did notice the lack of xml files which are referenced in the earlier bug, though adding them back didn't do anything. Just figured I would try the easiest solution first.

In the de-xbl effort we're doing, the xbl (.xml) are usuallyl converted to custom elements (.js)..

Using the developer tools debugger to debug by setting break points can be handy. Or just adding dump statements in the javascript (outputs like printf into the console). E.g. dump("xxxbenamin hehehhey\n");

I have a working fix, will upload patch once I've removed the debug code and add a comment or two.

Attached patch bug-1530213-fix.patch (obsolete) — Splinter Review

Let me know if it works, I only removed a few lines and tweaked one or two others.

Attachment #9067665 - Flags: review?(mkmelin+mozilla)
Attachment #9067665 - Flags: review?(jorgk)
Attachment #9067665 - Flags: feedback+

The patch is empty. Do a |hg qref| before attaching it.

Attachment #9067665 - Attachment is obsolete: true
Attachment #9067665 - Flags: review?(mkmelin+mozilla)
Attachment #9067665 - Flags: review?(jorgk)
Attachment #9067665 - Flags: feedback+

Well that's irritating, I checked the patch before exporting it. Should I be following the workflow at the bottom of. https://developer.thunderbird.net/contributing/fixing-a-bug/using-mercurial-queues ?

Attached patch bug-1530213-fix.patch (obsolete) — Splinter Review

Okay, this one has actual data in it.

Comment on attachment 9068021 [details] [diff] [review]
bug-1530213-fix.patch

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

The anonid="input" used to be https://dxr.mozilla.org/comm-esr60/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.xml#36

So we should instead of removing all the checks, check target.classList.contains("textbox-addressingWidget")

For commit messages, please use a format like so:
Bug 1530213 - make mailing lists used in the invitee dialog expand again.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +933,4 @@
>          }
>  
>          let roleStatusIcon = listitem.querySelector(".status-icon");
> +	if(roleStatusIcon) {

what's this for? it doesn't look correct

@@ +951,4 @@
>       */
>      resolvePotentialList(input) {
>          let fieldValue = input.value;
> +        if (fieldValue.length > 0) {

while here, we usually only check truthy, like
if (fieldValue) {

@@ +955,5 @@
>              let mailingList = this._resolveListByName(fieldValue);
>              if (mailingList) {
>                  let entries = this._getListEntries(mailingList);
>                  if (entries.length > 0) {
> +                    let currentIndex = this.mMaxAttendees - 2;

wow the old code is pretty here, carving out the number from something like attendeeCol3#1 without even a comment ... 
but the change also seems wrong
Status: NEW → ASSIGNED
Attached patch bug-1530213-fix2.patch (obsolete) — Splinter Review
  • Added comments to explain the code changes.
  • Changed the description to match the bug.
  • Added the check suggested by Magnus Melin
Attachment #9068021 - Attachment is obsolete: true
Comment on attachment 9068221 [details] [diff] [review]
bug-1530213-fix2.patch

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

When a patch is ready, you should flag it for review. You do that by setting the review ? flag on the attachment, and in the field that appears you select a reviewer by email address.
Patches without flags risk getting no attention.
For WIP patches, use the feedback flag instead.

I can confirm the patch works, but it still needs some adjustment.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +933,4 @@
>          }
>  
>          let roleStatusIcon = listitem.querySelector(".status-icon");
> +	// An error occurs if "roleStatusIcon" is null checking if it exists fixes the issue.

but it should never be null...
if it is, find out why

@@ +933,5 @@
>          }
>  
>          let roleStatusIcon = listitem.querySelector(".status-icon");
> +	// An error occurs if "roleStatusIcon" is null checking if it exists fixes the issue.
> +	if(roleStatusIcon) {

for code style this would have been: if (roleStatusIcon)

And you'd also indent the part inside the if

@@ +951,4 @@
>       */
>      resolvePotentialList(input) {
>          let fieldValue = input.value;
> +	// input.id is no longer a variable,   

These kind of comments you can leave in the bug, but if you put them in the code that would be confusing to future readers.
Only put comments about current state in the code.

Also, there's some trailing spaces here

@@ +956,5 @@
>              let mailingList = this._resolveListByName(fieldValue);
>              if (mailingList) {
>                  let entries = this._getListEntries(mailingList);
>                  if (entries.length > 0) {
> +		    // sense id is no longer set we have to use the length of the list as our guide. Subtracting by 2 fixes both the offset (have to count by zero) and the actual location of the list item we want to replace.

Please no comments on what used to be, only the current state. You could note down that index 0 is the organizer, then the rest is maybe as clear as it needs to be

@@ +957,5 @@
>              if (mailingList) {
>                  let entries = this._getListEntries(mailingList);
>                  if (entries.length > 0) {
> +		    // sense id is no longer set we have to use the length of the list as our guide. Subtracting by 2 fixes both the offset (have to count by zero) and the actual location of the list item we want to replace.
> +                    let currentIndex = this.mMaxAttendees -2;

nit: space around the -

For changed files, run the linter, like
./mach lint comm/calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js

@@ +962,3 @@
>                      let template = this.querySelector(".addressingWidgetItem");
>                      let currentNode = template.parentNode.childNodes[currentIndex];
>                      this._fillListItemWithEntry(currentNode, entries[0], currentIndex);

I notice _fillListItemWithEntry doesn't actually take a 3rd argument anymore... so fix the callers.

Gotcha, will fix and resubmit.

I've cleaned up the code, but haven't figured out why the first item on the list always returns null when setting roleStatusIcon. I'll keep working on it till I understand whats going on, even if the fix isn't forthcoming.

Attached patch bug-1530213-fix.patch (obsolete) — Splinter Review

Code has been cleaned up per your guidance.

roleStatusIcon is still returning null on the first list item (the one that is replaced when the list is expanded). So the code to get around that issue is still in the patch.

I need some guidance on where to look next for the possible culprit. The odd part of the issue is that the first item returns null, but subsequent "cloned" items do not.

I'll keep looking into it.

Attachment #9068221 - Attachment is obsolete: true
Attachment #9069377 - Flags: review?(mkmelin+mozilla)

Jorg informed me that we don't like tabs. I'll reformat and resubmit.

Comment on attachment 9069377 [details] [diff] [review]
bug-1530213-fix.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +933,4 @@
>          }
>  
>          let roleStatusIcon = listitem.querySelector(".status-icon");
> +	if (roleStatusIcon) {

If you look up how status-icon is used, you'll find that is strange. 
If set, its class will at some places be changed to role-icon. But since we're looking up by class, that won't work after such changes. 

I wonder if it wouldn't make sense to always just use the role-icon.
https://searchfox.org/comm-central/rev/e0c2c4a00a839e66b733dff1671cb18f1b217db3/calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js#719-724

It's a rare occasion that the organizer would not be participating, so having that info there seems not needed. (If that's what the data is put there for, I didn't look to verify.)
Comment on attachment 9069377 [details] [diff] [review]
bug-1530213-fix.patch

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

Ok, looking at it some more, let's go with this and file a follow-up for the role-icon, status-icon wrongness.
It looks like they should use the same thing as itip-icon where the role and status are overlayed on the icon. That's how it's showing in the event participants area.
Also need a folloup for the list size overflowing vertically now... (but that's a different bug.)
Attachment #9069377 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
Attachment #9069377 - Flags: approval-calendar-beta?(philipp)
Comment on attachment 9069377 [details] [diff] [review]
bug-1530213-fix.patch

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

I'm not landing this.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +936,5 @@
> +	if (roleStatusIcon) {
> +        	roleStatusIcon.removeAttribute("disabled");
> +        	roleStatusIcon.setAttribute("class", "role-icon");
> +        	roleStatusIcon.setAttribute("role", newAttendee.role);
> +	}

This is mis-indented since it's still using tabs.

@@ +955,5 @@
>              let mailingList = this._resolveListByName(fieldValue);
>              if (mailingList) {
>                  let entries = this._getListEntries(mailingList);
>                  if (entries.length > 0) {
> +                    let currentIndex = this.mMaxAttendees - 2; // start currentIndex at the begining of the address list

Can you move this comment onto the line before and really explain what is happening. If you have 10 attendees, you start at 8?

@@ +970,4 @@
>                          currentIndex++;
>                      }
>                      this.mMaxAttendees += entries.length;
> +                    for (let i = currentIndex; i <= this.mMaxAttendees - 1; i++) {

Why did that change? Also, why not use `i < this.mMaxAttendees` as for regular loops.
Attachment #9069377 - Flags: approval-calendar-beta?(philipp)

Magnus, what's happening with that index business here? Also, don't you want a Calendar peer to look at this?

Flags: needinfo?(mkmelin+mozilla)
Keywords: checkin-needed
Comment on attachment 9069377 [details] [diff] [review]
bug-1530213-fix.patch

Geoff, can to take a look at this please.
Attachment #9069377 - Flags: review?(geoff)
Comment on attachment 9069377 [details] [diff] [review]
bug-1530213-fix.patch

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

I'm giving this r- for the reasons Jorg gave, and a comment from me.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +933,4 @@
>          }
>  
>          let roleStatusIcon = listitem.querySelector(".status-icon");
> +	if (roleStatusIcon) {

You could do querySelector(".role-icon, .status-icon") which would pick up either. I don't think there's any state where the image doesn't have one of those two classes, so the if check is probably unnecessary.
Attachment #9069377 - Flags: review?(geoff) → review-

(In reply to Geoff Lankow (:darktrojan) from comment #25)

You could do querySelector(".role-icon, .status-icon") which would pick up
either. I don't think there's any state where the image doesn't have one of
those two classes, so the if check is probably unnecessary.

That's a bit of nonsense though, since in the if clause you set the role-icon class...
But like I wrote earlier, there's a bunch of wrongness going on with that.

Flags: needinfo?(mkmelin+mozilla)
Attached patch 1530213.patch (obsolete) — Splinter Review

Sorry for taking so long to get back to this.

I've gotten rid of the offending tabs and added the suggested change by Geoff in comment 25. It fixes the issue with roleStatusIcon returning null so all good there.

I also made the comment more clear per Jorg's request. And changed <= to < in the loop. The loop now looks like this.

for (let i = currentIndex; i < this.mMaxAttendees - 1; i++) {

I believe the - 1 in the loop will make sense with the new comment. It all amounts to the index no longer being supplied by the input, and having to use the mMaxAttendees variable instead.

Attachment #9075794 - Flags: feedback?(jorgk)
Attached patch 1530213.patch (obsolete) — Splinter Review

Sorry for taking so long to get back to this.

I've gotten rid of the offending tabs and added the suggested change by Geoff in comment 25. It fixes the issue with roleStatusIcon returning null so all good there.

I also made the comment more clear per Jorg's request. And changed <= to < in the loop. The loop now looks like this.

for (let i = currentIndex; i < this.mMaxAttendees - 1; i++) {

I believe the - 1 in the loop will make sense with the new comment. It all amounts to the index no longer being supplied by the input, and having to use the mMaxAttendees variable instead.

Attachment #9069377 - Attachment is obsolete: true
Attachment #9075794 - Attachment is obsolete: true
Attachment #9075794 - Flags: feedback?(jorgk)
Attachment #9075795 - Flags: feedback?(jorgk)
Comment on attachment 9075795 [details] [diff] [review]
1530213.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +953,5 @@
>              let mailingList = this._resolveListByName(fieldValue);
>              if (mailingList) {
>                  let entries = this._getListEntries(mailingList);
>                  if (entries.length > 0) {
> +                     // We set the current index based on the contact list items location in the attendees list

Nit: Full stop missing at end of line.

@@ +956,5 @@
>                  if (entries.length > 0) {
> +                     // We set the current index based on the contact list items location in the attendees list
> +                     // The index isn't given to us through input so we use mMaxAttendees instead
> +                     // subtracting one to count from 0.
> +                     // To replace the contact list entry with the first value in the list we subtract another.

You mean "mailing list"? And "member of the mailing list"?

@@ +971,5 @@
>                          this._fillListItemWithEntry(currentNode, entry, currentIndex);
>                          currentIndex++;
>                      }
>                      this.mMaxAttendees += entries.length;
> +                    for (let i = currentIndex; i < this.mMaxAttendees - 1; i++) {

I'm totally confused now:
Original code:
i <= this.mMaxAttendees;
Previous patch:
currentIndex; i <= this.mMaxAttendees - 1
This patch:
currentIndex; i < this.mMaxAttendees - 1
Attachment #9075795 - Flags: feedback?(jorgk)

Sorry yes mailing list is what I meant. I'll update the comments with proper punctuation and the word changes.

...
This patch:
currentIndex; i < this.mMaxAttendees - 1

It seems that having the - 1 there has no affect on the code even with the < instead of <= so I will remove it.

My initial thought was that by subtracting one from mMaxAttendees it would prevent the loop from running one too many times. Of course that's what removing the "=" would do as well.

I'll do some proofreading and up load an edited patch.

Attached patch 1530213.patchSplinter Review

Reworded the comment to hopefully make it more clear.

Removed the - 1 from the loop as it was no longer needed.

Attachment #9075795 - Attachment is obsolete: true
Attachment #9076001 - Flags: review?(jorgk)
Comment on attachment 9076001 [details] [diff] [review]
1530213.patch

Thanks, this looks OK to me now. I'm not a Calendar peer, so formally I can't review this. Geoff has looked at this before, so he'll review it now.
Attachment #9076001 - Flags: review?(jorgk)
Attachment #9076001 - Flags: review?(geoff)
Attachment #9076001 - Flags: feedback+

Gotcha, thanks for looking at it Jorg.

Comment on attachment 9076001 [details] [diff] [review]
1530213.patch

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

Okay, but please fix up the comment.

::: calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
@@ +956,5 @@
>                  if (entries.length > 0) {
> +                     // We set currentIndex based on the mailing list item location using mMaxAttendees with an offset of -2.
> +                     // The first: So that mMaxAttendees starts counting from 0.
> +                     // The second: To replace the mailing list entry with the first member in the mailing list.
> +                    let currentIndex = this.mMaxAttendees - 2;

This comment is indented one space too many, and the first line should be split in two. Plus I think I'd rewrite it to something like this:

"Start replacing rows at the currently active row, which is the second-to-last row that isn't a dummy row. (mMaxAttendees being a count of non-dummy rows.)"
Attachment #9076001 - Flags: review?(geoff) → review+
Attachment #9076001 - Flags: approval-calendar-beta?(philipp)

A bit disappointing that the patch doesn't apply at all:

$ hg -v qpush
applying 1530213.patch
patching file calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
bad hunk #1 @@ -927,53 +927,57 @@ class MozCalendarEventAttendeesList exte
(54 53 57 57)

Looks like you edited it manually and didn't adjust those numbers :-(

EDIT: Yes, it needs to be @@ -927,53 +927,56 @@ class MozCalendarEventAttendeesList exte
since you're adding three lines not four.

// Start replacing rows at the currently active row, which is the
// second-to-last row that isn't a dummy row. Note that mMaxAttendees
// is a count of non-dummy rows.

Sorry, this is is still not clear, so I had to try it out.

With myself as organiser, another invitee and then the list being replaced on position 3 (counting from 1), I saw a value of 4, so I really have no idea what this is counting. Also, what's a dummy row?

The patch works although it's still not clear to me why we're doing this:

-                    for (let i = currentIndex; i <= this.mMaxAttendees; i++) {
+                    for (let i = currentIndex; i < this.mMaxAttendees; i++) {

Was that wrong before?

I came to land that, but I can't :-(

Flags: needinfo?(geoff)

It appears that was wrong before.

mMaxAttendees (which does appear to be an odd name) is the count of rows that contain an icon and a space to accept an email address (or the organiser, I guess, which doesn't accept an email address, but has one). All of the other rows are dummy rows and only contain the bottom border. The bottom borders don't actually line up across the window, but that's a story for another bug.

Flags: needinfo?(geoff)

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2f0f868999d8
Make invite attendees expand mailing lists. r=darktrojan DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED

Landed with more accurate comment as discussed with Geoff on IRC.

Target Milestone: --- → 7.1

Bug 1565999 and bug 1565992 filed for the problems discovered while reviewing this.

Attachment #9076001 - Flags: approval-calendar-beta?(philipp)
You need to log in before you can comment on or make changes to this bug.