Closed Bug 313948 Opened 19 years ago Closed 16 years ago

Keep readOnly attribute set by user persist between sessions

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ssitter, Assigned: Fallen)

References

Details

Attachments

(3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051026 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051026 Mozilla Sunbird/0.2+

If the user explicit sets the readOnly attribute for a calendar this attribute is reset the next time Sunbird starts (Due to bug 313489).

If the readOnly attribute is set by user the attribute should persist between sessions.

If the readOnly attribute is set by an error the attribute should not persist between sessions. (As supplied before).

Reproducible: Always

Steps to Reproduce:
n.a.
Use persistent + non-persistent read-only flag. I know that the stuff in get readOnly() could be shorter but I prefer it the explicit way.
Attachment #200931 - Flags: first-review?(jminta)
imo, the problem isn't big enough to put this in 0.3a1.
(In reply to comment #2)
> imo, the problem isn't big enough to put this in 0.3a1.> 

I agree with you. So please keep this bug as an enhancement for 0.3a2 or later.
Comment on attachment 200931 [details] [diff] [review]
Use persistent + non-persistent read-only flag.

Apologies for the long delay in reviewing this.  While the code looks essentially correct, I have a problem with the way this solution is implemented.  You and I would know that to make a readOnly state persist, you should set it separately as a pref, but that's nowhere in the documentation, and new coders would have no easy way of discovering this.  Furthermore, I can easily forsee someone wanting to add observers to the readOnly interface in the calICalendar implementation.  But setting readOnly via a pref would completely bypass this.

Instead, my ideal solution would be to add a second argument to setting the readOnly flag, a boolean for whether or not it should persist.  The documentation in calICalendar.idl should then be updated accordingly.
Attachment #200931 - Flags: first-review?(jminta) → first-review-
Is this the right way to go?(In reply to comment #4)
> Instead, my ideal solution would be to add a second argument to setting the
> readOnly flag, a boolean for whether or not it should persist.  The
> documentation in calICalendar.idl should then be updated accordingly.

Driven by the comment above I would propose the following changes to interface calICalendar:

   /**
    * Is this calendar read-only?  Used by the UI to decide whether or not 
    * widgetry should allow editing.
+   *
+   * Returns true if either the non-persistent read-only flag in calendar
+   * or the persistent read-only flag in calendar preferences is true.
    */
-  attribute boolean readOnly;
+  readonly attribute boolean readOnly;
+
+  /**
+   * Set read-only state of calendar.
+   * 
+   * @param aReadOnlyFlag    New value for readOnly property.
+   * @param aPersitentFlag   Keep readOnly property between sessions.
+   *
+   * - If aPersitentFlag is true the value of aReadOnlyFlag will be
+   *   stored in persistent read-only flag in calendar preferences.
+   *
+   * - If aPersitentFlag is false the value of aReadOnlyFlag will be
+   *   stored in non-persistent read-only flag in calendar.
+   */
+  void setReadOnly( in boolean aReadOnlyFlag, in boolean aPersitentFlag );

What do you think? Waiting for comments.
(In reply to comment #5)
A couple quick comments that have come to me in the last few minutes:

1.) I think the descriptions need to include more about a 'session'.

2.) We need a clear picture of what happens in an over-ride scenario, ie:
   -cal.setReadOnly(true, true);
   -cal.setReadOnly(false, false);
   -restart
Intuitively, the calendar should remain as read-only after the restart.  In this sense, it might be better to characterize aPersistentFlag as:
  -if true, sets the calendar's default readonly attribute to the value of aReadOnlyFlag.  The calendar will take on this value as its readonly attribute at the start of each session
  -if false, over-rides the calendar's read-only attribute with aReadOnlyFlag for this session only.  After restarting, the previous value (whether the same or different) will again be the calendar's read-only attribute.

More importantly, I think we're going to need to seriously consider how we present these cases to the user.  I know I would be very confused if sometimes my calendar stayed read-only and sometimes it didn't.  This part may depend on a much better analysis of the errors we get in calICSCalendar and pass off to the errorPrompt.
It might be clearer to split the readonly flag in two flags: readOnly and inErrorState.

readOnly would be set by the user, using a checkbox in the edit-calendar dialog. It would persist between sessions.

inErrorState would be set by the calendar provider, when it encounters an error. The edit-calendar dialog would have a button to clear the error, to unset the flag. It would not persist.
I decided to spend some more time on this feature. I agree with Michiel that a second property would be the appropriate solution.

Fixing the providers and the code using readOnly does not seem like a big problem to me. But the question that came into my mind was: How do we explain it to the user that there are two Read-Only modes, a persitent and a non-persistent one.

After careful consideration I came up with the following proposal:

(A) Normal property dialog without error: The wording of the read-only mode has been updated to reflect the persitent of that option.

:                                                   :
|  Location: [____________________________________] |
|                                                   |
|            [ ] Always open in Read-Only mode      |
|                                                   |
|---------------------------------------------------|
|                                 [ OK ] [ Cancel ] |
'---------------------------------------------------'

(B) Property dialog after error: Hidden content is shown giving some basic explanations to the user. Below is a big button to remove the Read-Only mode.

:                                                   :
|  Location: [____________________________________] |
|                                                   |
|            [ ] Always open in Read-Only mode      |
|                                                   |
|---------------------------------------------------|
|                                                   |
|  An error happened when reading the calendar.     |
|  To prevent further damage the calendar has been  |
|  placed in Read-Only mode for current session.    |
|                                                   |
|    [Reset Read-Only mode for current session]     |
|                                                   |
|---------------------------------------------------|
|                                 [ OK ] [ Cancel ] |
'---------------------------------------------------'

If the user presses the Reset button the property is cleared and I could imagine two ways to give feedback:
- Hide the content so that the dialog is displayed as in (A) or
- Show another text like 'The calendar is editable again.' and disable the button.

Comments?
When will this functionality be restored : i.e. the possibility of having a persistent read-only flag, represented as a "READ-ONLY" pref ?

I tried getting/setting a "READ-ONLY" pref in the get/set readOnly() functions but it causes a "too much recursion error".
Assignee: mostafah → base
Component: Sunbird and Calendar-Extension Front End → Base
OS: Windows 2000 → All
QA Contact: sunbird → base
Hardware: PC → All
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
Whiteboard: [ca ui-review needed]
What are the possible motivations for a user wanting to set a calendar as read-only by hand instead of having the program detect and do so automatically?
Whiteboard: [ca ui-review needed] → [cal ui-wanted]
(In reply to comment #11)
> What are the possible motivations for a user wanting to set a calendar as
> read-only by hand instead of having the program detect and do so automatically?

A sysadmin who is too lazy to setup web-dav permissions properly for several shared calendars might make use of this functionality (like me, for instance).
(In reply to comment #11)
> What are the possible motivations for a user wanting to set a calendar as
> read-only by hand instead of having the program detect and do so automatically?

As you most likely know, Lightning does not currently detect when a user can't write to a webdav calendar.  This causes the disturbing behavior of updating the calendar on the screen but the changes are lost when the user restarts Lightning.  There are already bugs for that (e.g. Bug 316941 and bug 341501).  Perhaps fixing those bugs would make this bug unnecessary?

In any case, my current workaround for those bugs would be to manually mark those calendars as read-only, if this persisted between sessions.

Ultimately, my preference is that Lightning/Sunbird automate all of this.  What if they did something like the following every time that they tried to write to a calendar:

1) read the latest version of the calendar from the server
2) update it with the user's change
3) write the calendar to the server
4) read it again from the server to verify that it worked
5) if it didn't work then report the error and make the calendar read-only for the session.

It seems that Lightning does most of this now except perhaps for steps (4) and (5).
(In reply to comment #13)
> 5) if it didn't work then report the error and make the calendar read-only for
> the session.

On second thought, I think that it would be better if Lightning/Sunbird reported the error but did *not* make the calendar read-only.  Why?  First, if the user didn't know that the calendar was read-only, he knows now and won't make the same mistake twice.

Second, if it's a problem with the network or the calendar server, it's probably a temporary problem, so why force the user to later manually disable the read-only status of the calendar?  The user will know when the network/server is back online the next time that he tries to update the calendar or use any internet applications on that network.  So it seems that Lightning/Sunbird don't have to do anything in this regard except to report the error when it happens.

Maybe one can compare it to how Thunderbird works.  If a user does a manual mail check, Thunderbird reports any error but doesn't go into offline mode or anything.
I have been looking at this problem as we have a situation where we have multiple calendars in a single subdirectory. We want users to be able to view all the calendars, but not necessarily edit all of them. Not because it's "wrong", but just to prevent accidents. What we have found is that because all the calendars are written and read by Apache, you cannot set user permissions for an individual iCal file, only prevent that user from accessing the directory. The "read only" flag would be a very useful feature in this situation.

The only other way I can come up with to solve this is to have a subdirectory for each user, complete with apache permissions for just that user, and apache would have full permission. There would then be a central folder, to which apache only had read-only access, with simlinks to all the individual iCal files (or perhaps vice-versa). The user would use this central folder for read only access, and their own folder for access to their own calendars. This would remove a lot of flexibility form the original solution.

Not really a nice solution, and not easy to manage. I therefore believe that this "Read Only" functionality is very important.
I agree with Marco van Beek. (In reply to comment #15)

The read-only functionality is a very useful feature when sharing iCal files via HTTP (Apache). It is very likely that you move or delete somebody's calendar events accidentally. One wrong click would be enough...

So is there a solution yet?

By the way: the read-only flag is not secure because the user can switch it off and edit somebody's calendar. Is it possible to prevent the user from editing the read-only flag?
Keywords: uiwanted
Whiteboard: [cal ui-wanted]
You show the reason why making the read-only switch be sticky is not a good solution in your own comment: is it not secure. The only way to get it seucre is to manage it at the server level. Read the apache documentation. I'm pretty sure you can get user-level permissions, without a separate directory for each user. Don't use the filesystem permissions, but let apache do it all.
(In reply to comment #17)

Yes, it's not secure but very, very useful! We need shared calendars where everybody must have read access to the other one's calendars. Only the own calendar should be writeable per default.

BUT: If someone asks you to edit an event in his/her calendar (e.g. by phone) you're able to take out the read-only flag temporarily, edit the calendar and make it read-only again. The read-only flag is useful in trusted areas and networks.

Sometimes easy is better than complicated...
I have been looking for ages and have never found any reference to a per file per user permissions system within Apache, only per directory. DAV relies on Apache permissions, hence the horrible bodge described above.

I am wondering if iCal via WebDAV is just too limited for it to be a long term solution. 
I just today came across a good reason to implement this:

I am subscribed to some Google Calendars via ICS feed, since I do not want to accidentally edit them. Every time I have one of those calendars selected and want to quickly create an event on my local storage calendar, I get a readonly notification (at least in the current session). If the readonly flag would persist and additionally creating new events happens in the first writable calendar instead of the selected calendar, I would not have to care too much about what I have selected.
(In reply to comment #19)
> I have been looking for ages and have never found any reference to a per file
> per user permissions system within Apache, only per directory. DAV relies on
> Apache permissions, hence the horrible bodge described above.
> 
> I am wondering if iCal via WebDAV is just too limited for it to be a long term
> solution. 
> 

I have successfully implemented this with the following Apache directives:

<Directory /www/htdocs/calendar/ted>
        DAV On
        AuthType Basic
        AuthUserFile /www/secure/htaccess/passwd
        AuthGroupFile /www/secure/htaccess/group
        AuthName "Secure Area - HAHA"
        Require user Ted.Milker
        <Files ted.ics>
                Require valid-user
                <LimitExcept GET HEAD OPTIONS>
                        Require group ted
                </LimitExcept>
        </Files>
</Directory>

Which basically lets authenticated users subscribe to my public ted.ics calendar to view only but not any other calendars in the folder.  I used groups just so if I want to give someone else the ability to modify my public calendar, I can.

The only reason why I want a session persistent read only flag is because of other bugs in Lightning(GET but not PUT permission bug) causing havoc.  If the other bugs went away, the persistent read only flag would not be necessary.
Well, I guess I should not say that is the *only* reason.  The other reason is that Lightning will attempt to create an event with whichever calendar you have selected in the Calendars tab.  The persistent read only flag is a good way(red warning message and cannot click OK) to prevent me from adding things to the wrong calendars that I may have write access to.  It would still be useful if that bug was fixed.
I just found another reason for wanting a "Read Only" flag, albeit as a workaround for a more serious bug, which is that Sunbird NEVER locks a file (in WebDAV) before updating, so you have to be really careful about having multiple people accessing the same calendar, who need to be able to edit it occasionally, but not all the time. See bug 290446
I had filed bug 392757 under the impression that this bug here is on remote calendars only, not local ones. Sorry for the duplicate if the underlying mechanism is the same. While the "Read Only" attribute might be redetermined for a remote calendar, it definitely cannot for a local one. Thus, retaining this attribute between sessions would be of major importance for local calendars.
Two another reasons for a persistent read-only flag

1. Usability: The user can set/reset this option so it has to be persistant (or you have to inform the user _why_ the option has the current state ... seems to be expensive).

2. I connect to eGroupware via iCal. There are some ... hm ... incompatibilties between Sunbird and eGW while syncing (who ever has the bug). But the read only mode works for me.
What motivation is behind having two readOnly flags? If an error occurs, the user is told that the calendar is put into readOnly mode. If the users wants to edit his calendar again, its his choice to open the calendar props and remove the readonly flag. If he on the other hand the keeps the calendar in readOnly mode and closes calendar and opens again, what speaks against having the readOnly flag still being set?

Given bug 379174 will add an icon to the calendar list, it will be easy to set/unset and notice the readOnly mode.

This of course is just a suggestion, I don't want to bypass any comments or critique just by requesting review.

This patch takes care of making the readOnly attribute persist as soon as it is set. Notice I removed the setUpReadOnlyObservers() from calCalendarManager and added the readOnly mode observer in the getCalendars() function. This now means that the readOnly setters can call the calendar manager!

All providers are changed. I haven't tested caldav or wcap. Especially wcap wasn't trivial to change. Please test!

I also kept getting an error when a calendar was readOnly that some item could not be selected in the event dialog. I couldn't reproduce without this patch though. Looking at the code, I hope I fixed it. Comments welcome.
Attachment #277706 - Flags: review?(daniel.boelzle)
(In reply to comment #27)
> What motivation is behind having two readOnly flags? If an error occurs,
> the user is told that the calendar is put into readOnly mode.

See Bug 313489 for the motivation to make read-only-after-error non-persistent.
Comment on attachment 277706 [details] [diff] [review]
Use only persistant read-only flag

[I see only one (admittedly minor) restriction now, i.e. we couldn't set readOnly on unregistered calendars anymore.]

>Index: calendar/providers/caldav/calDavCalendar.js
>+    // attribute boolean readOnly;
>     get readOnly() { 
>-        return this.mReadOnly;
>+        return getCalendarManager().getCalendarPref(this, "READONLY");
>     },
>+
>     set readOnly(bool) {
>-        this.mReadOnly = bool;
>+        if (bool) {
>+            getCalendarManager().setCalendarPref(this, "READONLY", true);
>+        } else {
>+            getCalendarManager().deleteCalendarPref(this, "READONLY");
>+        }
>+        return bool;
>     },
and the like
- With respect to moving calendar management to the prefs, shouldn't we switch to lower-cased identifiers, e.g. "readonly"?
- setCalendarPref() takes a string argument. I suspect that calling with true coerces to an empty string which is bad style. IMO (again with respect to moving it to the prefs) we should change calICalendarManager::get|set|deleteCalendarPref to work with variant values (but document that only string, boolean and integer are supported).

>Index: calendar/providers/memory/calMemoryCalendar.js
>-    // Most of the time you can just let the ics calendar handle this
>+    // attribute boolean readOnly;
>     get readOnly() { 
>-        return this.mReadOnly;
>+        return getCalendarManager().getCalendarPref(this, "READONLY");
>     },
>+
>     set readOnly(bool) {
>-        this.mReadOnly = bool;
>+        if (bool) {
>+            getCalendarManager().setCalendarPref(this, "READONLY", true);
>+        } else {
>+            getCalendarManager().deleteCalendarPref(this, "READONLY");
>+        }
>+        return bool;
>     },
Memory calendars aren't registered, so why make readOnly persistent?
Attachment #277706 - Flags: review?(daniel.boelzle) → review-
Attachment #200931 - Attachment is obsolete: true
Since the calendar manager code already landed elsewhere, updated patch with the rest of the stuff addressed. I hope I did the WCAP part right.
Assignee: nobody → philipp
Attachment #277706 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #280815 - Flags: review?(daniel.boelzle)
Since this has a patch, I'd like it for 0.7 :)
Flags: blocking-calendar0.7?
Flags: blocking-calendar0.7?
I just wanted to file a bug for this, instead I voted for this.

I would expect a check box the "read only" to be editable and persist. If the write access was disable due to an error, I would expect an non-editable text that informs me about this situation, with the check box "read only" being checked. I could accept this (may be sensible) or correct the write access and set the calendar writeable again.

If I have calendar which is updated via an other program (e.g. fetch it daily), be it remote or local (file://), I'd really like to permanently mark it as read only. It is my free will not to be able to edit this calender, but now Lightning ignores my will at a restart. 

Summary: Persistent "read only" flag, on error set "read only" and inform permanently.
I'd like to bring in another approach which makes sense to me:
I think it's noncontentious that setting a calendar read-only should be made persistent. In case of an error, we should prompt the user to make the calendar read-only (instead of enforcing read-only), e.g. the error box could generally recommend a read-only checkbox. My point is that the user needs to explicitly acknowledge (or reject) making it read-only, and doesn't wonder why it's read-only after restart.
(In reply to comment #33)
See Comment #8 for a slightly similar proposal but in the Properties dialog. 
Maybe the extended Error Notification proposed in Comment #33 could use a similar solution.
Seriously, why hasn't this been fixed yet?  It has to be one of the most annoying bugs I've had to deal with in Lightning.  I have several google calendars that belong to friends and need to be read-only.   It's incredibly irritating to have to set each one of them as read-only every time I restart Thunderbird, and dealing with accidentally creating events for those calendars, only to have it Lightning get upset, and set the calendar to read-only after creating the event.  This forces me to unset read-only, delete the event, which causes Lightning to get mad again and reset the read-only flag.
In the meantime you can use Persistent Read-Only Calendars extension
I have come across this bug with the latest stable Sunbird and the latest nightly (Calendar? but I *liked* Sunbird as a name) for OS X when connecting to wcap calendars.

In our situation there are people (personal assistants, secretaries, research fellows.. whatever) who will require to be able to edit the calendars of their managers. They can uncheck the "read-only" property for the wcap connected calendars and make edits all they want. Next time Sunbird is launched they're back to read-only.

I will try the extension mentioned in comment 36 but it seems to me that either I'm missing something (quite possible) or the way this value is being used is counterintuitive.

(In reply to comment #37)
WCAP provider is working different here. See Bug 395777 Comment #4.

It doesn't help that the extension is only available from the German version of the addons site (unless I'm mistaken).  The extension was not easy to get to.
Flags: wanted-calendar0.8+
Attachment #280815 - Attachment is obsolete: true
Attachment #280815 - Flags: review?(daniel.boelzle)
Why has the last patch here been obsoleted? It would be SO nice if this was fixed for 0.8!
I completely agree, this bug should be in 0.8, is so important for usability !
This bug is fixed with changes for bug 393395. Since it's now obvious that a calendar is readOnly (because of the padlock), there's no immediate need to reset that flag for next startup. However, we may consider to enhance the error dialog (e.g. adding a prechecked checkbox stating that the calendar will be put into readOnly mode).
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Version: Trunk → unspecified
Verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13pre)
Gecko/20080213 Lightning/0.8pre (2008021319) Thunderbird/2.0.0.13pre.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.