818688 - Commands that allow to create new events/tasks don't always reflect the calendars read-only status

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Decathlon]

The commands I'm talking about are:
calendar_new_event/todo_command
calendar_new_event/todo_context_command
calendar_new_todo_todaypane_command

The bug is "visible" in the UI by observing the status disabled/enabled of the buttons "New Event" and "New Task" in the main toolbar that directly observe those commands (or the items in the menu File > New > Event[Task])


Steps to reproduce:
Given two calendars A and B
1. set the calendar A in read-only mode, then do the same for the calendar B
   -> the buttons New Event/Task get disabled as they should;
2. change the read-only property for the calendar A (the first one changed to
   read-only)
   -> the buttons are still disabled but they shouldn't because now there is
      a writable calendar.
3. select the calendar B then again the calendar A
   -> now the status of the buttons depends on the selected calendar despite
      the fact that there is always a writable calendar.
   
Reproducible: always.

Comment 1

[Decathlon]

As far as I can see this bug depends on the global variable CalendarNewItemsCommandEnabled in the file calendar-common-sets.js, but since its meaning it's not completely clear to me, I kindly ask for Wolfgang Sourdeau's opinion since he added that variable in the patch for bug 586276.

That boolean is being changed by the function calendarUpdateNewItemsCommand()
http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-common-sets.js#934

but its value depends on the read-only/writable status of a single calendar and not on the status of *all* the calendars. It is being changed when a calendar is selected.

Since the commands calendar_new_event/todo_command depends directly from this variable:

http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-common-sets.js#108
http://mxr.mozilla.org/comm-central/source/calendar/base/content/calendar-common-sets.js#122

we have a logic AND between variables that reflect the status of all the calendars and a variable that depends on the status of one calendar (the selected one).

Deleting CalendarNewItemsCommandEnabled from the two lines mentioned above fixes the bug, but maybe there's something that I missed since I don't fully understand its meaning. Maybe should it work differently?

Comment 2

[Martin Sch]

See also https://bugzilla.mozilla.org/show_bug.cgi?id=825833

Comment 4

[Decathlon]

This bug is a regression from Bug 586276 (or maybe from bug 714431 that fixed another regression from the same bug).

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Decathlon from comment #1)
> As far as I can see this bug depends on the global variable
> CalendarNewItemsCommandEnabled in the file calendar-common-sets.js, but
> since its meaning it's not completely clear to me, I kindly ask for Wolfgang
> Sourdeau's opinion since he added that variable in the patch for bug 586276.
That variable is used mostly for the ACL manager. A calendar can have an ACL manager that decides if its always readonly or not, if users can maybe just modify and not create new events, etc.

The manager is not fully integrated unfortunately, not all UI makes use of the manager, but we do disable some commands.

The default ACL manager should be advertising this variable as always true, which would make it depend on the user's choice.

Does that shed more light into the situation? I haven't checked the code, so if you still think it should be removed please say so and I'll take a closer look.

Comment 7

[[:MakeMyDay]]

Attachment: ReflectCreateItemCommandStatusBasedOnEffectiveAccess-V1.patch

I have investigated on this bug in correlation with my reports on bugs 948967, 944683 and 949444 and created a first patch.

This patch

1) adds checks whether a calendar supports events and tasks to calUtil.jsm (or should this be kept in calendar-common-sets.js or moved to calUtils.js?),
2) takes care to perform the ACL checks in calendarUpdateNewItemsCommand() on all calendars and not just the selected one,
3) splits CalendarNewItemsCommandEnabled into CalendarNewEventsCommandEnabled and CalendarNewTasksCommandEnabled to perfom the check on each possible combination of isCalendarWritable(cal), userCanAddItemsToCalendar(cal) and item type (which was not the case before),
4) deprecates calendars_support_tasks() and calendars_support_calendars() (or should this be removed directly? The functions are not used anymore after relying on CalendarNewEventsCommandEnabled and CalendarNewTasksCommandEnabled inside isCommandEnabled)

For notice:
If we make use of the isCalendarWritable check inside the get writable() function of the commandConroller too, we can deprecate or remove some not further required helpers used exclusively inside the commandController: get all_readonly(), get has_local_calendars(), get has_cached_calendars(), get all_local_calendars_readonly()

Additionally, I'm not sure whether the further occurences of this.writeable checks in calendar-common-sets.js should take the ACL stuff into account as well. But to keep the patch on this bug small, we may want to adress the writable-realted staff into another bug?

Comment 8

[[:MakeMyDay]]

I forgot to mention that even the menu controls are enabled/disabled correctly with this patch, the target calendar, which is set when crearting the new event/task dialog still relies on the selected calendar. Maybe it would be useful to have another function beside getSelectedCalendar() that tries to determine the best target calendar in a more sophisticated way (considering the effectively writable calendars, one of the itip-calendars tied to the users standard mail account, selected calendar, ...). But to keep things simple, I think this should also be for a separate bug.

Comment 9

[[:MakeMyDay]]

Attachment: ReflectCreateItemCommandStatusBasedOnEffectiveAccess-V2.diff

Updated patch fixing two typos and adjusting patch format.

Any chance to get a first review on this soon?

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8357081 [details] [diff] [review]
ReflectCreateItemCommandStatusBasedOnEffectiveAccess-V2.diff

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

Can you make sure the patch still works with the double return fixed? I'll give it a whirl afterwards, r- to get a new patch.

::: calendar/base/content/calendar-common-sets.js
@@ +514,5 @@
>       * Returns a boolean indicating if its possible to write items to any
>       * calendar.
>       */
>      get writable() {
> +        return return (getCalendarManager().getCalendars({}).filter(isCalendarWritable).length > 0);

Double return, use cal prefix for getCalendarManager and isCalendarWritable.

@@ +602,5 @@
>      /**
>       * Returns a boolean indicating that at least one of the calendars supports
>       * tasks.
> +     * 
> +     * @deprecated use (getCalendarManager().getCalendars({}).filter(isTaskCalendar) > 0) instead

Any deprecated functions here can be removed, I doubt these are being used by extensions because they just control the view commands.

@@ +954,5 @@
> +    // re-calculate command status
> +    CalendarNewEventsCommandEnabled = false;
> +    CalendarNewTasksCommandEnabled = false;
> +    let calendars = getCalendarManager().getCalendars({}).filter(isCalendarWritable).filter(userCanAddItemsToCalendar);
> +    if (calendars.filter(cal.isEventCalendar).length > 0)

if (calendars.some(cal.isEventCalendar)) {

same with other places you use filter and length > 0

::: calendar/base/modules/calUtils.jsm
@@ +148,5 @@
> +     * Checks whether a calendar supports tasks
> +     * 
> +     * @param aCalendar
> +     */
> +    isTaskCalendar: function cal_isTaskCalendar(aCalendar) {

A few whitespaces need cleaning up here.

Comment 11

[[:MakeMyDay]]

Attachment: ReflectCreateItemCommandStatusBasedOnEffectiveAccess-V3.diff

the updated patch addresses your comments. I have no idea why there came in the double return in the last version of the patch - simply removing is safe though.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8393187 [details] [diff] [review]
ReflectCreateItemCommandStatusBasedOnEffectiveAccess-V3.diff

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

r=philipp

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v4

Patch for checkin with all headers, tree is still closed.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/32a293b64afb