874156 - Selecting an alarm notification for an event that was edited after the notification fires causes a broken calendar UI to appear

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Calendar, defect)

Description

[Jason Smith [:jsmith]]

Attachment: Broken Calendar UI

Build: B2G 18 5/20/2013
Device: Unagi

STR

1. Create an offline event that will fire soon
2. Wait for the event to fire to show the alarm notification
3. After the notification appears, change the start and end time of the event to something distinctly different
4. Exit the calendar app
5. Select the event alarm notification

Expected

The event the alarm fired on should appear.

Actual

See screenshot. We get a broken calendar UI in this case.

Comment 4

[buri.blff]

Dear Jason:
  please update status.

Comment 5

[Jason Smith [:jsmith]]

(In reply to buri.blff from comment #4)
> Dear Jason:
>   please update status.

The bug isn't fixed. That's the current status.

Comment 7

[Peipei Cheng (needinfo if you need my action)]

This bug is still reproducible on Buri V1.2. 
Gaia:     ce276842c9ac1746073271fb736dfdb626a89240
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/36c4c667b9f2 
BuildID   20131121004002                                                     
Version   26.0   

V1.1 has the same problem.

Comment 8

[James Zhang (Spreadtrum)]

Attachment: 874156.patch

Fugu and buri also have this bad UE issue.
Please review the patch and fix it, thanks.

Comment 9

[Jason Smith [:jsmith]]

Gareth could probably help you with the review on this patch.

Comment 10

[Gareth Aye [:gaye] (back from PTO)]

Thanks for flagging me jsmith. I will go ahead and r? myself so that it's in my queue for this week.

Comment 11

[Jesse]

Attachment: calendar_event_not_found.patch

Try the new patch. Sorry for tabs in former patch.

Comment 12

[Kevin Grandon :kgrandon]

Comment on attachment 8336719 [details] [diff] [review]
874156.patch

Obsoleting first patch.

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8342148 [details] [diff] [review]
calendar_event_not_found.patch

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

The main problem I can see with this patch is that it does not follow the expected STR from Jason. If we are going to do something like this, we should first check with QA/Product to ensure that this is an acceptable approach. Due to the badness of the UI, I suppose navigating to the month view is ok for a quick fix.

In any case, we should definitely have some kind of unit test for this before landing. Information on unit tests can be found here: https://github.com/mozilla-b2g/gaia/#unit-tests

If you need help with a unit test, or need someone to take over the patch and write a unit test, feel free to ping me and we can figure something out.

Comment 14

[Kevin Grandon :kgrandon]

Jason - How would you feel about a short term fix to simply display the month view if we are unable to load the event from the notification? Implementing redirects would be non-trivial and may not make 1.3. I would recommend displaying the month view for 1.3, and we can work on something more concrete for 1.4+.

Comment 15

[Jesse]

Kevin, thanks for your review. We would like a 'quick' fix in the 'fugu' project. Thanks!

Comment 16

[Jason Smith [:jsmith]]

(In reply to Kevin Grandon :kgrandon from comment #14)
> Jason - How would you feel about a short term fix to simply display the
> month view if we are unable to load the event from the notification?
> Implementing redirects would be non-trivial and may not make 1.3. I would
> recommend displaying the month view for 1.3, and we can work on something
> more concrete for 1.4+.

I don't think I'd recommend going for the short term fix here. It feels like were fixing this bug by replacing it with another bug that has it's own complications. The user is still confused at this point on why their calendar event didn't load. There's also the regression risk of what this workaround introduces - could we accidentally load the month view in a situation that a user didn't expect it to load in?

Comment 17

[Kevin Grandon :kgrandon]

(In reply to Jason Smith [:jsmith] from comment #16)
> (In reply to Kevin Grandon :kgrandon from comment #14)
> I don't think I'd recommend going for the short term fix here. It feels like
> were fixing this bug by replacing it with another bug that has it's own
> complications. The user is still confused at this point on why their
> calendar event didn't load. There's also the regression risk of what this
> workaround introduces - could we accidentally load the month view in a
> situation that a user didn't expect it to load in?

This is true, but I do feel like a redirect to the month view is a much safer bet than showing a broken UI. 

I'm going to do a needsinfo? on James here to see if he has any suggestions that may make this easier than I am thinking it may be.

Comment 18

[James Zhang (Spreadtrum)]

This bug is still on v1.3.

Comment 19

[Jason Smith [:jsmith]]

Not a regression, so this is not a blocker.

Comment 20

[BMO Automation]

Firefox OS is not being worked on