The default bug view has changed. See this FAQ.

Alert if storage db version is newer than we know how to deal with

VERIFIED FIXED in Sunbird 0.5

Status

Calendar
Provider: Local Storage
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Matthew (lilmatt) Willis, Assigned: Matthew (lilmatt) Willis)

Tracking

Trunk
Sunbird 0.5

Details

(Whiteboard: [verified0.3.1])

Attachments

(2 attachments, 4 obsolete attachments)

27.78 KB, image/png
Details
12.58 KB, patch
Matthew (lilmatt) Willis
: first-review+
Michiel van Leeuwen (email: mvl+moz@)
: second-review+
Matthew (lilmatt) Willis
: ui-review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 242947 [details] [diff] [review]
Adds alert.
Assignee: nobody → lilmatt
Status: NEW → ASSIGNED
Attachment #242947 - Flags: ui-review?(dmose)
Attachment #242947 - Flags: second-review?(jminta)
Attachment #242947 - Flags: first-review?(cmtalbert)
(Assignee)

Comment 2

11 years ago
Created attachment 242951 [details]
alert screenshot

Comment 3

11 years ago
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).
Attachment #242947 - Flags: first-review?(cmtalbert) → first-review+

Comment 4

11 years ago
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

11 years ago
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)
(Assignee)

Comment 6

11 years ago
Comment on attachment 242947 [details] [diff] [review]
Adds alert.

ui-r=dmose via phone

Clearing r2?jminta per comment 5
Attachment #242947 - Flags: ui-review?(dmose)
Attachment #242947 - Flags: ui-review+
Attachment #242947 - Flags: second-review?(jminta)
(Assignee)

Comment 7

11 years ago
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.
Attachment #242947 - Attachment is obsolete: true
Attachment #243809 - Flags: ui-review+
Attachment #243809 - Flags: second-review?(jminta)
Attachment #243809 - Flags: first-review+
(Assignee)

Comment 8

11 years ago
Created attachment 243811 [details]
New alert

Screenshot of new alert.
Attachment #242951 - Attachment is obsolete: true
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.
(Assignee)

Comment 10

11 years ago
Created attachment 243818 [details]
per mvl

screenshot of alert with mvl's request to remove the "continue and hose my data" button
Attachment #243811 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #243818 - Attachment mime type: text/plain → image/png
(Assignee)

Comment 11

11 years ago
Created attachment 243821 [details] [diff] [review]
updated patch with changes per mvl

Now with "Verbage-by-beltzner" TM
Attachment #243809 - Attachment is obsolete: true
Attachment #243821 - Flags: ui-review+
Attachment #243821 - Flags: second-review?(mvl)
Attachment #243821 - Flags: first-review+
Attachment #243809 - Flags: second-review?(jminta)
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
Attachment #243821 - Flags: second-review?(mvl) → second-review+
(You shouldn't carry forward ui-review+ flags if in next patches you change the ui)
(Assignee)

Comment 14

11 years ago
Upgrading piece removed. That is bug 333688.

Patch with nits addressed checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Blocks: 333688
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: blocking-calendar0.3.1?
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
Flags: blocking-calendar0.3.1? → blocking-calendar0.3.1+
(Assignee)

Comment 16

10 years ago
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]
Whiteboard: [fixed0.3.1]

Comment 17

10 years ago
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

10 years ago
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

10 years ago
(In reply to comment #18)

I think this will be followed up by Bug 369819 now.
(Assignee)

Comment 20

10 years ago
I'll add a relnote for 0.3.1 as well.

-> VERIFIED
Status: RESOLVED → VERIFIED
Whiteboard: [fixed0.3.1] → [verified0.3.1]
You need to log in before you can comment on or make changes to this bug.