MagicMirrorOrg / MagicMirror

MagicMirror² is an open source modular smart mirror platform. With a growing list of installable modules, the MagicMirror² allows you to convert your hallway or bathroom mirror into your personal assistant.
http://magicmirror.builders
MIT License
19.7k stars 4.2k forks source link

Issue: Calendar setting does not override global setting #963

Closed QNimbus closed 7 years ago

QNimbus commented 7 years ago

Platform: Raspberry Pi 3

Node Version: node v8.2.1

MagicMirror Version: MagicMirror v2.1.3-dev

Description:

In the calendar module documentation it states that the settings maximumEntries and maximumNumberOfDays override the global setting (defined in the root of the module config). In practice this only works for the maximumNumberOfDays setting - the maximumEntries does not override the global maximumEntries setting.

Steps to Reproduce:

Define a calendar where you specify the maximumEntries and maximumNumberOfDays both on the module level and on the calendar level. When this config is used, the calendar specific maximumNumberOfDays setting correctly overrides the global maximumNumberOfDays setting, however the calendar specific maximumEntries setting does not override the global setting. (see sample config below. The calendar correctly applies the maximum of 56 days, but it only shows 4 maximum entries instead of the expected 6 as the module documentation implies.

Expected Results:

With the config below the calendar should show 6 entries within the 56 day period.

Actual Results:

With the config below the calendar only shows 4 entries within the 56 day period.

Configuration:

        {
            module: "calendar",
            position: "top_left",
            header: "<i class=\"fa fa-plane\" aria-hidden=\"true\"></i>Schedule",
            config: {
                titleReplace: {
                    NIZ: "Niet inzetbaar",
                    OSV: "Ouderschaps verlof",
                    KOV: "Vrij (korte opdracht)",
                },
                excludedEvents: ["Reisverlof", "EN ROUTE", "AANMELDEN", "AFMELDEN"],
                maximumEntries: 4,
                maximumNumberOfDays: 4,
                calendars: [
                    {
                        symbol: "",
                        fadePoint: 0.3,
                        maximumEntries: 6,
                        maximumNumberOfDays: 56,
                        url: "https://calendar.google.com/calendar/ical/[censored]/basic.ics"
                    }
                ]
            }
        },

Additional Notes:

I'm working on a PR to address this issue - I will also add some test cases in the very near future to prevent regression. However I'm strapped for time at the moment so this will come at a later time

qistoph commented 7 years ago

Commit https://github.com/MichMich/MagicMirror/commit/7bd256c311a47ad552f4c6b4bbafe381c719dd0f introduces another issue with my configuration.

I have a calendar module instance with two calendar sources. After this commit (found using bisect) my calendar shows 20 items even though I use the default maximumEntries: 10.

roramirez commented 7 years ago

@qistoph If it reverses the commit works well?. We can create a test for this?

qistoph commented 7 years ago

Reversing the commit makes it the expected 10 items again. That's what git bisect helps you with. "Use binary search to find the commit that introduced a bug"

We could probably come up with a test to check this.

QNimbus commented 7 years ago

@qistoph I will write a test case to properly test this - in the meantime, can I see the specific part of your config.js file (the calendar part) to see how you configured your calendar? Take care to remove any private/personal URLS.

qistoph commented 7 years ago

Sure and thanks!

{
  module: 'calendar',
  header: 'Agenda',
  position: 'top_left',
  config: {
    timeFormat: 'absolute',
    dateFormat: 'ddd D MMM',
    fullDayEventDateFormat: 'ddd D MMM',
    urgency: 0,
    calendars: [
      {
        symbol: 'calendar-check-o',
        url: 'https://calendar.google.com/calendar/ical/REMOVED'
      },
      {
        symbol: 'birthday-cake',
        url: 'webcal://www.facebook.com/ical/b.php?REMOVED'
      }
    ]
  }
},
qistoph commented 7 years ago

For clarity; I would want to show the first 10 events from the merged calendars. E.g: Appointment 1 Appointment 2 Birthday 1 Birthday 2 Appointment 3 Appointment 4 Appointment 5 Birthday 2 Appointment 6 Appointment 7

There is no predefined number of Facebook birthdays or Google Calendar events I want to show, but a grand total of 10 max.

So on second thought, it seems the override of maximumEntries seems to collide with this. I feel necessity for two variables;

Any updates from your side?

QNimbus commented 7 years ago

@qistoph You're correct, I've started writing some test cases to properly test the calendar configuration and the global 'maxEntries' setting is overridden by the calendar specific 'maxEntries' setting.

So at the moment the calendar module is working exactly as documented (with regard to the maxEntries config setting).

I agree with you that it makes more sense to have a total maximum for the calendar module and a maximum number of entries per calendar source. But this would require a change to the calendar module source and the documentation of the calendar module configuration. If everyone agrees that this would make more sense than the current implementation I could make a PR for this change?

MichMich commented 7 years ago

Closed due to inactivity (and since it's not really an issue.) Feel free to reopen.