1016687 - [B2G][Calendar] - Event icons / indicators (dots below the date) are still visible in month view after disabling offline calendar

Status: VERIFIED FIXED

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

Description

[Joshua Mitchell (Inactive)]

Description:
When disabling the offline calendar any events from it should not be visible. After adding events and then disabling the calendar the event indicators (a dot below the day # for each event) are still shown. Additionally, days that held an event prior to disabling the offline calendar will not have the "No 
Events" text in the the events detail section.

Repro Steps:
1) Update a Open_C to BuildID: 20140527013003
2) Select Calendar
3) Add an event
4) Select the drawer in the upper left corner
5) Select offline calendar to disable it (check mark is removed)
6) Select the drawer in the upper left corner to return to the calendar page
7) Select the day the previously created even was on (while in month view)

Actual:
Day has event indicators (dots) below the date number.

Expected:
Event indicators will be removed when the offline calendar is turned off.

Environmental Variables:
Device: Open_C 2.0
BuildID: 20140527013003
Gaia: 6a391274cd436f8f0d1fad2db8c6b4805703259c
Gecko: cbe4f69c2e9c
Version: 32.0a1
Firmware Version: P821A10V1.0.0B06_LOG_DL

Environmental Variables:
Device: Buri 2.0 MOZ
BuildID: 20140526232421
Gaia: 6a391274cd436f8f0d1fad2db8c6b4805703259c
Gecko: cbe4f69c2e9c
Version: 32.0a1
Firmware Version: v1.2-device.cfg

Repro frequency: 100%


Notes: This bug was requested through the comments on https://bugzilla.mozilla.org/show_bug.cgi?id=1009210

Comment 1

[Jason Smith [:jsmith]]

This needs a screenshot.

Comment 2

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Attachment: Calendar still shows dots and does not show "No Events" text

Attaching a screenshot per Jason's request.

Comment 3

[Jason Smith [:jsmith]]

So I can reproduce this. From what I can see, the dots appear to show up for all calendars no matter what - the calendar disabling doesn't play a factor here. This sounds like a bug, but let me double check with UX.

UX - What's the expected behavior here with the dot indicators on the month view?

Comment 4

[Stephany Wilkes]

Appears to be a bug but flagging Harly on Calendar.

Comment 5

[Harly Hsu[:harly]]

Miller, is this related to the structure of our calendar design as well like in week/day view? Is there a way to fix this so that the dots can refresh to only indicating the events without the disabled calendar?

Comment 6

[Miller Medeiros [:millermedeiros]]

Harly, do we have a clear user story for the dots? I'm thinking about potential edge-cases, like if you have events from multiple calendars on the same day, etc..

The current calendar structure isn't very smart about toggling the calendar visibility (it simply hides the elements). We have many edge cases with the dots, if there are events from other calendars active we would need to keep the dots. Maybe I'll need to change the whole logic and trigger a full re-render of the view every time the calendars are toggled (that would also fix other problems but isn't a trivial change).

Comment 7

[Jason Smith [:jsmith]]

In the old calendar UX, disabling a calendar technically removed the events from being viewable in each calendar view. With the new calendar UX, this bug reveals this isn't implemented for the month view. That means we're missing parity with the old calendar UX in the month view and missing a feature.

Wilfred - Can you confirm this is a requirement of the new calendar UX in 2.0 (i.e. should this have feature-b2g set to 2.0)? It seems strange to me that we would introduce a new UX that removes a use case from the old calendar UX.

Comment 8

[Harly Hsu[:harly]]

Hey Miller,
The user story for dots are quite simple, just display the number of the events in the event list (including all day events), and only show 3 dots if there are more than 3 events. 

Hi Jason, there is no differences between the old &new calendar UX. I think what UX wants is clear that the calendar should refresh each view when enable/disable calendars. Thanks

Comment 9

[Jason Smith [:jsmith]]

(In reply to Harly Hsu from comment #8)
> Hey Miller,
> The user story for dots are quite simple, just display the number of the
> events in the event list (including all day events), and only show 3 dots if
> there are more than 3 events. 
> 
> Hi Jason, there is no differences between the old &new calendar UX. I think
> what UX wants is clear that the calendar should refresh each view when
> enable/disable calendars. Thanks

There is definitely a difference in the old & new calendar UX here. When I disabled a calendar in the old UX, the events don't show in the month view. The new calendar UX doesn't do this in the month view, which is a broken user story in parity with the old calendar UX.

Comment 10

[Miller Medeiros [:millermedeiros]]

[TL;DR] I did a quick and dirty test and decided that the "best thing" is to do it "the right way", which will also fix a couple other bugs along the way (eg. Bug 986335). That also means I'll need to ask Gareth to review next week and agree with my solution (which might take a while).

Just to describe the problem and what I'm planning to change:

Right now we toggle the display by updating the CSS rules and adding a `display:none` to the class that matches the given calendar (elements with classes `.calendar-{calendarId}.calendar-display` will toggle display every time we change the calendar visibility). - This means we are still adding all the events/busytimes to the views, but we are just not displaying them (that's why the dots still use old events count and why when calendar is not displayed the events still take up space when they overlap).

What I plan to do is to actually "filter" the events/busytimes inside the the `TimeController` to only return busytimes from calendars that are "active" and redraw the current "time frame" whenever the calendars are toggled. (and remove the old logic, which is a huge mess/hack)

It is probably not a trivial change (affects multiple views and some of the core components of the app), so I *might* introduce new bugs. It should also not be so easy to review (since changes will probably span thru multiple files),

PS: I already fixed a few bugs related to the current calendar toggle logic, so doing it "the right way" even if it takes a couple extra days will probably pay off in the long run. - if I knew we would have so many bugs related to that I would have changed the logic when I fixed Bug 969472 or Bug 982240. - the current logic is from 2012, the requirements probably changed a lot since then.

Comment 11

[Jason Smith [:jsmith]]

I'm marking qawanted here to clarify what happened in 1.4 here to validate my comments above.

Comment 12

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19837

my patch is still work in progress.. but at least we can start the review earlier. I introduced a few bugs along the way but I'm almost sure I addressed all of them and in the end I did not need to change that much code to get it to work.

Gareth, please let me know if you agree with my solution so that I can move forward and update the tests. - I also think we should open a new bug later to kill the whole calendar_colors.js file.

please let me know if you guys find anything "weird" (regressions) with my patch.

Comment 13

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

I like the approach very much! Nice work Miller :). I would, of course, like some integration tests that demonstrate the ways this was broken and that it is now fixed.

Food for thought: this would be *so* easy if we used knockoutjs :P

Comment 14

[Miller Medeiros [:millermedeiros]]

since we need to write integration tests for this new behavior I'll need to fix the toggle calendar tests before landing this patch.

Comment 16

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

Comment on attachment 8431897 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19837

Miller,

This is a beautiful patch. I thoroughly enjoyed reading through. Nice simplification and great test coverage. Well done!

Comment 17

[Miller Medeiros [:millermedeiros]]

landed on master: https://github.com/mozilla-b2g/gaia/commit/afa912907429c2067a71ed46d9d49d61dd040111

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing around bug 1007519. It might be easiest to just uplift that as well (along with Julien's disabling).

Comment 19

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request [v2.0]: https://github.com/mozilla-b2g/gaia/pull/20943

rebased over v2.0 branch and also uplifted the toggle_calendar_test.js commits (one to update and another one to disable it)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8445504 [details] [review]
Link to Github pull-request [v2.0]: https://github.com/mozilla-b2g/gaia/pull/20943

I don't need to review it :). I'm about to head out for the day, but I'll check the Travis results in the morning and merge the PR if everything looks OK :)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/4bb9a5c68308ca35f2d6d6d18eb2b0eeaae147e1

Comment 22

[Edward Chen[:edchen]]

[Environment]
Gaia      2248c0367661db9332f70f37055e1a8176f5f612
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/44d31566a3a6
BuildID   20140629160202
Version   32.0a2
ro.build.version.incremental=108
ro.build.date=Tue Jun 10 19:40:40 CST 2014

[Result]
Pass

Comment 23

[Sue]

Attachment: video of issue verify

This issue has been verified successfully on Flame v2.0 & v2.1
See attachment: verify_video.MP4
Reproducing rate: 0/5
Flame 2.0 versions:
Gaia-Rev        f9d6e3d83c3922e9399a6c27f5ce4cdd27bdfd05
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/45112935086f
Build-ID        20141126000203
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.032754
FW-Date         Wed Nov 26 03:28:05 EST 2014
Bootloader      L1TC00011880

Flame 2.1 versions:
Gaia-Rev        db2e84860f5a7cc334464618c6ea9e92ff82e9dd
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/211eae88f119
Build-ID        20141126001202
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141126.033519
FW-Date         Wed Nov 26 03:35:30 EST 2014
Bootloader      L1TC00011880