Last Comment Bug 357458 - Alert if storage db version is newer than we know how to deal with
: Alert if storage db version is newer than we know how to deal with
Status: VERIFIED FIXED
[verified0.3.1]
:
Product: Calendar
Classification: Client Software
Component: Provider: Local Storage (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Sunbird 0.5
Assigned To: Matthew (lilmatt) Willis
:
:
Mentors:
Depends on:
Blocks: 333688
  Show dependency treegraph
 
Reported: 2006-10-20 18:46 PDT by Matthew (lilmatt) Willis
Modified: 2007-02-09 09:17 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Adds alert. (3.36 KB, patch)
2006-10-20 18:47 PDT, Matthew (lilmatt) Willis
cmtalbert: first‑review+
mattwillis: ui‑review+
Details | Diff | Splinter Review
alert screenshot (27.16 KB, image/png)
2006-10-20 20:21 PDT, Matthew (lilmatt) Willis
no flags Details
Now dies. (11.66 KB, patch)
2006-10-27 11:18 PDT, Matthew (lilmatt) Willis
mattwillis: first‑review+
mattwillis: ui‑review+
Details | Diff | Splinter Review
New alert (31.90 KB, image/png)
2006-10-27 11:19 PDT, Matthew (lilmatt) Willis
no flags Details
per mvl (27.78 KB, image/png)
2006-10-27 11:43 PDT, Matthew (lilmatt) Willis
no flags Details
updated patch with changes per mvl (12.58 KB, patch)
2006-10-27 11:51 PDT, Matthew (lilmatt) Willis
mattwillis: first‑review+
mvl: second‑review+
mattwillis: ui‑review+
Details | Diff | Splinter Review

Description Matthew (lilmatt) Willis 2006-10-20 18:46:32 PDT
Currently, if we find a storage db with a newer version number than our max version number we just go happily along, possibly doing something bad to the db.

We should alert the user.
Comment 1 Matthew (lilmatt) Willis 2006-10-20 18:47:54 PDT
Created attachment 242947 [details] [diff] [review]
Adds alert.
Comment 2 Matthew (lilmatt) Willis 2006-10-20 20:21:46 PDT
Created attachment 242951 [details]
alert screenshot
Comment 3 cmtalbert 2006-10-24 16:01:30 PDT
Comment on attachment 242947 [details] [diff] [review]
Adds alert.

This looks fine. My only nit is that several of the lines extend past the 80 character mark. It appears that calStorageCalendar.js does not provide Cc and Ci const definitions, so spelling those out is consistent with the rest of the file (and a major source of the > 80 char line trouble).
Comment 4 Joey Minta 2006-10-24 18:06:47 PDT
Comment on attachment 242947 [details] [diff] [review]
Adds alert.

This looks OK in general.  However, I wonder if we want something along the lines of toolkit's NS_ASSERT.  That is, a generic function that all debug-esque prompts pass through, that can be disabled at release time.
Comment 5 Joey Minta 2006-10-26 13:52:13 PDT
Comment on attachment 242947 [details] [diff] [review]
Adds alert.

After IRC discussion, we decided it would probably be much better if OK actually shut down the app for you (with a cancel option if you really want to try to force the table to work)
Comment 6 Matthew (lilmatt) Willis 2006-10-26 14:14:27 PDT
Comment on attachment 242947 [details] [diff] [review]
Adds alert.

ui-r=dmose via phone

Clearing r2?jminta per comment 5
Comment 7 Matthew (lilmatt) Willis 2006-10-27 11:18:41 PDT
Created attachment 243809 [details] [diff] [review]
Now dies.

The best thing to do is apply this patch with the patch from bug 333688 (schema update) since it requires that to land first or at the same time.
Comment 8 Matthew (lilmatt) Willis 2006-10-27 11:19:50 PDT
Created attachment 243811 [details]
New alert

Screenshot of new alert.
Comment 9 Michiel van Leeuwen (email: mvl+moz@) 2006-10-27 11:29:30 PDT
I don't see any need for the 'ignore' button. Just remove it, and maybe replace it with some hidden pref or whatever. If somebody sees this error, he will first press the default button. Then he notices that it doesn't solve his problem, and he will very likely click the ignore button, and loose all his data. That's very, very bad. We should make it less easy to do wrong things like that.
Comment 10 Matthew (lilmatt) Willis 2006-10-27 11:43:36 PDT
Created attachment 243818 [details]
per mvl

screenshot of alert with mvl's request to remove the "continue and hose my data" button
Comment 11 Matthew (lilmatt) Willis 2006-10-27 11:51:27 PDT
Created attachment 243821 [details] [diff] [review]
updated patch with changes per mvl

Now with "Verbage-by-beltzner" TM
Comment 12 Michiel van Leeuwen (email: mvl+moz@) 2006-10-29 07:02:11 PST
Comment on attachment 243821 [details] [diff] [review]
updated patch with changes per mvl

THere is w ahole lot of code before this that doesn't belong in this patch. Get rid of it.

>+            } else if (version > this.DB_SCHEMA_VERSION) {

Add a comment here, explaining what's going to happen. r=mvl with that fixed.

>+tooNewSchemaErrorTitle=Your data isn't compatible with this version of %1$S
>+tooNewSchemaErrorText=The data in your profile was updated by a newer version of %1$S, and continuing will probably cause the information to be lost or corrupted.

Add some more text, saying something like 'Sunbird will now close'  ui-review=mvl with that fixed.

>+tooNewSchemaButtonQuit=Quit %1$S
Comment 13 Michiel van Leeuwen (email: mvl+moz@) 2006-10-29 07:03:47 PST
(You shouldn't carry forward ui-review+ flags if in next patches you change the ui)
Comment 14 Matthew (lilmatt) Willis 2006-10-29 08:58:50 PST
Upgrading piece removed. That is bug 333688.

Patch with nits addressed checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Comment 15 Dan Mosedale (:dmose) 2007-01-24 15:01:29 PST
We want a version of this without the schema upgrade for 0.3.1.  Not having it seems too scary.  A partial diff is at:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=calCalendarManager.js&branch=MOZILLA_1_8_BRANCH&root=/cvsroot&subdir=mozilla/calendar/base/src&command=DIFF_FRAMESET&rev1=1.20.2.9&rev2=1.20.2.10
Comment 16 Matthew (lilmatt) Willis 2007-01-26 09:09:15 PST
Appropriate bits checked in for 0.3.1

LIGHTNING_0_3_BRANCH:
Checking in locales/en-US/chrome/calendar/calendar.properties;
/cvsroot/mozilla/calendar/locales/en-US/chrome/calendar/calendar.properties,v  <--  calendar.properties
new revision: 1.60.2.9.2.1; previous revision: 1.60.2.9
done
Checking in base/src/calCalendarManager.js;
/cvsroot/mozilla/calendar/base/src/calCalendarManager.js,v  <--  calCalendarManager.js
new revision: 1.20.2.9.2.1; previous revision: 1.20.2.9

SUNBIRD_0_3_BRANCH:
Checking in locales/en-US/chrome/calendar/calendar.properties;
/cvsroot/mozilla/calendar/locales/en-US/chrome/calendar/calendar.properties,v  <--  calendar.properties
new revision: 1.68.2.1; previous revision: 1.68
done
Checking in base/src/calCalendarManager.js;
/cvsroot/mozilla/calendar/base/src/calCalendarManager.js,v  <--  calCalendarManager.js
new revision: 1.32.2.1; previous revision: 1.32

Marking [fixed0.3.1]
Comment 17 Stefan Sitter 2007-01-27 02:43:18 PST
There is an issue when running with Thunderbird + Lightning: 

The message shows "Your data isn't compatible with this version of Thunderbird. The data in your profile was updated by a newer version of Thunderbird, and continuing will probably cause the information to be lost or corrupted."

There is no information that this is about your calendar data and Lightning. I think this will mislead users.
Comment 18 cmtalbert 2007-02-04 05:28:42 PST
Verified that it works on Sunbird, but I agree with Stefan that the behavior on Lightning is confusing.  However, this is such a fairly small set of users that I think we might be able to get by with a release note.  

So we have two choices: 

Fix lightning: -->REOPEN?

Release Note: --> VERIFIED?

Comment 19 Stefan Sitter 2007-02-09 01:55:33 PST
(In reply to comment #18)

I think this will be followed up by Bug 369819 now.
Comment 20 Matthew (lilmatt) Willis 2007-02-09 09:17:15 PST
I'll add a relnote for 0.3.1 as well.

-> VERIFIED

Note You need to log in before you can comment on or make changes to this bug.