Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix calendar not showing events with the same name and start date but different calendar url #3166

Merged
merged 6 commits into from
Aug 26, 2023

Conversation

Paranoid93
Copy link
Contributor

I fixed the calendar module, which did not show calendar entries from different calendars that share the same name and start date.

My use case: We have each office days documented in each an own calendar. If both "Office" Calendar entries start at the same date just one was shown

Google Calendar (each Office event in an own calendar)
260753381-c8d5aedf-3c11-4d91-83e8-8549eb261e58

Before
260751994-b308d549-fcb9-406e-9419-cdd2fed96dc6

After
260753208-3278e32b-9ca5-483a-bc6f-745cbf3964fc

@rejas
Copy link
Collaborator

rejas commented Aug 15, 2023

Thx for the PR, unfortunately your test is failing. Can you have a look?

@Paranoid93
Copy link
Contributor Author

I'm not that deep into the code. The better solution would probably be to compare some event ID. But for this use case the change should fix it

@rejas
Copy link
Collaborator

rejas commented Aug 15, 2023

please run npm run lint:prettier and commit the changes to make the tests run again

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2023

Codecov Report

Merging #3166 (a9701fe) into develop (156db32) will increase coverage by 0.71%.
Report is 5 commits behind head on develop.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff             @@
##           develop    #3166      +/-   ##
===========================================
+ Coverage    25.51%   26.23%   +0.71%     
===========================================
  Files           53       53              
  Lines        11501    11502       +1     
===========================================
+ Hits          2935     3018      +83     
+ Misses        8566     8484      -82     
Files Changed Coverage Δ
modules/default/calendar/calendar.js 0.00% <0.00%> (ø)

... and 4 files with indirect coverage changes

tests/configs/modules/calendar/multiple-calendars.js Outdated Show resolved Hide resolved
@@ -559,7 +559,7 @@ Module.register("calendar", {
let maxPastDaysCompare = now - this.maximumPastDaysForUrl(calendarUrl) * ONE_DAY;
for (const e in calendar) {
const event = JSON.parse(JSON.stringify(calendar[e])); // clone object

event.url = calendarUrl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why move this?

Copy link
Contributor Author

@Paranoid93 Paranoid93 Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is because otherwise the url isn't set yet for the comparison in listContainsEvent() in line 574.

But a better solution would be to compare events based on their UID. But for this I'd need more code (and javascript :D) knowledge

@Paranoid93 Paranoid93 requested a review from rejas August 16, 2023 18:24
@rejas
Copy link
Collaborator

rejas commented Aug 20, 2023

Sorry for the long delay until I commented more. Now I had the time to look at the code myself and I'd rather add a new config option hideDuplicate (which in your case you would set to true), use that as check before calling listContainsEvent and undo your change with the url comparison.
Reason for this: Checking if the url is the same is not really a check since that would mean you add an event from the same url: so why would you add the same url twice to the calendar anyway? If an event is duplicate it should mean that title, startdate are the same. A bonus to add would to also check for the same enddate (if present) becuase that is relevant over mutliple url calendars. I hope I make sense :-)

@rejas rejas merged commit 2ad463b into MagicMirrorOrg:develop Aug 26, 2023
Paranoid93 added a commit to Paranoid93/MagicMirror-Documentation that referenced this pull request Oct 11, 2023
rejas pushed a commit to MagicMirrorOrg/MagicMirror-Documentation that referenced this pull request Oct 16, 2023
* Updating calendar documentation

Updating regarding PR MagicMirrorOrg/MagicMirror#3166

* Update calendar.md

fix typo

* Update calendar.md

fix description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants