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

Update compliments with support for cron type date/time for selections, addition to just date. #3481

Merged
merged 23 commits into from
Jul 15, 2024

Conversation

sdetweil
Copy link
Collaborator

@sdetweil sdetweil commented Jun 24, 2024

  • What does the pull request accomplish? Use a list if needed.

this change allows uses to configure date/time events for compliments..
also linked site that will build the cron entry..

and example was Happy hour in a pub, on fri/sat between 5 and 7 pm.

or just after midnight on Halloween (Boooooo!)

I also added testcases for #3478

(and added support for this in MMM-Config), with a custom, drop down selection list of the types.. )

| if this is approved I will update the module doc

@sdetweil sdetweil closed this Jun 24, 2024
@sdetweil sdetweil reopened this Jun 25, 2024
@sdetweil
Copy link
Collaborator Author

the same failing 2 tests run correctly in the electron test suite.. what is the problem here?

copied from the electron test suite and added the get.document() call before the expect. but only these two fail.
like the date isn't set

@sdetweil
Copy link
Collaborator Author

ok, the problem here is the startApplication() helper in e2e does not accept dates for the test.. the electron version does accept date

so we cannot run date based testcases in e2e .. the electron tests DO pass

@sdetweil
Copy link
Collaborator Author

the failing testcase depends on the weather...

	it("should render a compliment based on the current weather", async () => {
    > 41 | 			await expect(weatherFunc.getText(".compliments .module-content span", "snow")).resolves.toBe(true);
         | 			      ^
      42 | 		});

this is a bad testcase.. but not related to my changes.

@sdetweil
Copy link
Collaborator Author

the weather testcase depends on a weather provider responding to the special value in the config

		{
			module: "weather",
			position: "bottom_bar",
			config: {
				location: "Munich",
				mockData: '"#####WEATHERDATA#####"'
			}
		}

but the default provider does not send any notification..

@sdetweil
Copy link
Collaborator Author

the failure is the weather test, untouched by me

  Received promise rejected instead of resolved
    Rejected to value: [Error: expect(received).toBe(expected) // Object.is equality·
    Expected: "snow"
    Received: ""]

      39 |
      40 | 		it("should render a compliment based on the current weather", async () => {
    > 41 | 			await expect(weatherFunc.getText(".compliments .module-content span", "snow")).resolves.toBe(true);
         | 			      ^
      42 | 		});
      43 | 	});

@khassel
Copy link
Collaborator

khassel commented Jun 26, 2024

untouched, but you changed compliment sources

@sdetweil
Copy link
Collaborator Author

yes, but that testcase is in weather.. not in compliments
and that part of compliments wasnt changed. it depends on the notification

	describe("Compliments Integration", () => {
		beforeAll(async () => {
			await weatherFunc.startApp("tests/configs/modules/weather/currentweather_compliments.js", {});
		});

		it("should render a compliment based on the current weather", async () => {
			await expect(weatherFunc.getText(".compliments .module-content span", "snow")).resolves.toBe(true);
		});
	});

i tested and the compliments notification_received is not triggered..

in compliments.js

		// Add compliments based on weather
		if (this.currentWeatherType in this.config.compliments) {
			Array.prototype.push.apply(compliments, this.config.compliments[this.currentWeatherType]);
			// if the predefine list doesn't include it (yet)             // i added these two lines
			if (!this.pre_defined_types.includes(this.currentWeatherType)) {
				// add it
				this.pre_defined_types.push(this.currentWeatherType);
			}
		}

		// Add compliments for anytime
		Array.prototype.push.apply(compliments, this.config.compliments.anytime);

the config for the weather_compliments testcase

let config = {
	modules: [
		{
			module: "compliments",
			position: "top_bar",
			config: {
				compliments: {
					snow: ["snow"]
				},
				updateInterval: 3000
			}
		},
		{
			module: "weather",
			position: "bottom_bar",
			config: {
				location: "Munich",
				mockData: '"#####WEATHERDATA#####"'
			}
		}
	]
};

doesn't have date based, and doesn't have normal ,, so depends on the notification, which doesn't arrive...

	// Override notification handler.
	notificationReceived (notification, payload, sender) {
		if (notification === "CURRENTWEATHER_TYPE") {
			this.currentWeatherType = payload.type;
		}
	}

I forced this.currentWeatherType to 'snow' with a compliment under "snow" and it appeared.. so the compliments module is not broken

@khassel
Copy link
Collaborator

khassel commented Jun 26, 2024

with old compliments.js:

node@cb3498f2205a:/opt/magic_mirror$ npx jest tests/e2e/modules/weather_current_spec.j --force-exit
 PASS   e2e  tests/e2e/modules/weather_current_spec.js (21.22 s)
  Weather module
    Current weather
      Default configuration
        ✓ should render wind speed and wind direction (814 ms)
        ✓ should render temperature with icon (201 ms)
        ✓ should render feels like temperature (200 ms)
        ✓ should render humidity next to feels-like (201 ms)
    Compliments Integration
      ✓ should render a compliment based on the current weather (5315 ms)
    Configuration Options
      ✓ should render windUnits in beaufort (304 ms)
      ✓ should render windDirection with an arrow (206 ms)
      ✓ should render humidity next to wind (204 ms)
      ✓ should render degreeLabel for temp (203 ms)
      ✓ should render degreeLabel for feels like (202 ms)
    Current weather with imperial units
      ✓ should render wind in imperial units (306 ms)
      ✓ should render temperatures in fahrenheit (201 ms)
      ✓ should render 'feels like' in fahrenheit (203 ms)

Test Suites: 1 passed, 1 total
Tests:       13 passed, 13 total
Snapshots:   0 total
Time:        21.85 s
Ran all test suites matching /tests\/e2e\/modules\/weather_current_spec.j/i.

with compliments.js of this PR:

node@cb3498f2205a:/opt/magic_mirror$ npx jest tests/e2e/modules/weather_current_spec.j --force-exit
 FAIL   e2e  tests/e2e/modules/weather_current_spec.js (6.252 s)
  Weather module
    Current weather
      Default configuration
        ✓ should render wind speed and wind direction (305 ms)
        ✓ should render temperature with icon (201 ms)
        ✓ should render feels like temperature (202 ms)
        ✓ should render humidity next to feels-like (202 ms)
    Compliments Integration
      ✕ should render a compliment based on the current weather (212 ms)
    Configuration Options
      ✓ should render windUnits in beaufort (308 ms)
      ✓ should render windDirection with an arrow (206 ms)
      ✓ should render humidity next to wind (200 ms)
      ✓ should render degreeLabel for temp (203 ms)
      ✓ should render degreeLabel for feels like (203 ms)
    Current weather with imperial units
      ✓ should render wind in imperial units (300 ms)
      ✓ should render temperatures in fahrenheit (202 ms)
      ✓ should render 'feels like' in fahrenheit (200 ms)

  ● Weather module › Compliments Integration › should render a compliment based on the current weather

    expect(received).resolves.toBe()

    Received promise rejected instead of resolved
    Rejected to value: [Error: expect(received).toBe(expected) // Object.is equality·
    Expected: "snow"
    Received: ""]

      39 |
      40 |              it("should render a compliment based on the current weather", async () => {
    > 41 |                      await expect(weatherFunc.getText(".compliments .module-content span", "snow")).resolves.toBe(true);
         |                            ^
      42 |              });
      43 |      });
      44 |

      at expect (node_modules/expect/build/index.js:113:15)
      at Object.expect (tests/e2e/modules/weather_current_spec.js:41:10)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 12 passed, 13 total
Snapshots:   0 total
Time:        6.297 s
Ran all test suites matching /tests\/e2e\/modules\/weather_current_spec.j/i.

@sdetweil
Copy link
Collaborator Author

looking.. how do you debug the code here?

@khassel
Copy link
Collaborator

khassel commented Jun 26, 2024

looking.. how do you debug the code here?

within the test? This is difficult, AFAIR only option is console.dir()

@sdetweil
Copy link
Collaborator Author

and how to debug..
i had to create the config.js used by the test after copying the mock data,
so I could test it in a real MM run..

@sdetweil
Copy link
Collaborator Author

updateInterval from testcases for #3471, I didn't check that ,, fixed
header.. oops I started with a prior version,, didn't catch that.. fixed

@sdetweil
Copy link
Collaborator Author

done

@khassel khassel requested a review from rejas June 27, 2024 21:24
@sdetweil
Copy link
Collaborator Author

sdetweil commented Jun 27, 2024

because I had to fix the prior testcases, it made me realize there is a cron anytime as well.. so I added that to e2e configs and tests

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jul 9, 2024

@rejas please review. I'd love your feedback

modules/default/compliments/compliments.js Show resolved Hide resolved
modules/default/compliments/compliments.js Outdated Show resolved Hide resolved
modules/default/compliments/compliments.js Outdated Show resolved Hide resolved
modules/default/compliments/compliments.js Outdated Show resolved Hide resolved
modules/default/compliments/compliments.js Outdated Show resolved Hide resolved
@sdetweil sdetweil requested a review from rejas July 13, 2024 18:02
@khassel khassel merged commit d9665b3 into MagicMirrorOrg:develop Jul 15, 2024
6 checks passed
@sdetweil sdetweil deleted the update_compliments branch July 15, 2024 18:04
@rejas rejas mentioned this pull request Sep 19, 2024
khassel pushed a commit that referenced this pull request Sep 19, 2024
- should now correct itself when one changes from (accidentaly selected)
master to develop
- also fixes wrong CHANGELOG entry from
#3481
- update deps a little
@khassel khassel mentioned this pull request Sep 30, 2024
khassel added a commit that referenced this pull request Sep 30, 2024
## [2.29.0] - 2024-10-01

Thanks to: @bugsounet, @dkallen78, @jargordon, @khassel,
@KristjanESPERANTO, @MarcLandis, @rejas, @ryan-d-williams, @sdetweil,
@skpanagiotis.

> ⚠️ This release needs nodejs version `v20` or `v22`, minimum version
is `v20.9.0`

### Added

- [compliments] Added support for cron type date/time format entries mm
hh DD MM dow (minutes/hours/days/months and day of week) see
https://crontab.cronhub.io for construction (#3481)
- [core] Check config at every start of MagicMirror² (#3450)
- [core] Add spelling check (cspell): `npm run test:spelling` and handle
spelling issues (#3544)
- [core] removed `config.paths.vendor` (could not work because `vendor`
is hardcoded in `index.html`), renamed `config.paths.modules` to
`config.foreignModulesDir`, added variable `MM_CUSTOMCSS_FILE` which -
if set - overrides `config.customCss`, added variable `MM_MODULES_DIR`
which - if set - overrides `config.foreignModulesDir`, added test for
`MM_MODULES_DIR` (#3530)
- [core] elements are now removed from `index.html` when loading script
or stylesheet files fails
- [core] Added `MODULE_DOM_UPDATED` notification each time the DOM is
re-rendered via `updateDom` (#3534)
- [tests] added minimal needed node version to tests (currently v20.9.0)
to avoid releases with wrong node version info
- [tests] Added `node-libgpiod` library to electron-rebuild tests
(#3563)

### Removed

- [core] removed installer only files (#3492)
- [core] removed raspberry object from systeminformation (#3505)
- [linter] removed `eslint-plugin-import`, because it doesn't support
ESLint v9. We will reenter it later when it does.
- [tests] removed `onoff` library from electron-rebuild tests (#3563)

### Updated

- [weather] Updated `apiVersion` default from 2.5 to 3.0 (#3424)
- [core] Updated dependencies including stylistic-eslint
- [core] nail down `node-ical` version to `0.18.0` with exception
`allow-ghsas: GHSA-8hc4-vh64-cxmj` in `dep-review.yaml` (which should
removed after next `node-ical` update)
- [core] Updated SocketIO catch all to new API
- [core] Allow custom modules positions by scanning index.html for the
defined regions, instead of hard coded (PR #3518 fixes issue #3504)
- [core] Detail optimizations in `config_check.js`
- [core] Updated minimal needed node version in `package.json`
(currently v20.9.0) (#3559) and except for v21 (no security updates)
(#3561)
- [linter] Switch to ESLint v9 and flat config and replace
`eslint-plugin-unicorn` by `@eslint/js`
- [core] fix discovering module positions twice after #3450

### Fixed

- Fixed `checks` badge in README.md
- [weather] Fixed issue with the UK Met Office provider following a
change in their API paths and header info.
- [core] add check for node_helper loading for multiple instances of
same module (#3502)
- [weather] Fixed issue for respecting unit config on broadcasted
notifications
- [tests] Fixes calendar test by moving it from e2e to electron with
fixed date (#3532)
- [calendar] fixed sliceMultiDayEvents getting wrong count and
displaying incorrect entries, Europe/Berlin (#3542)
- [tests] ignore `js/positions.js` when linting (this file is created at
runtime)
- [calendar] fixed sliceMultiDayEvents showing previous day without
config enabled

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Michael Teeuw <[email protected]>
Co-authored-by: Kristjan ESPERANTO <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ross Younger <[email protected]>
Co-authored-by: Veeck <[email protected]>
Co-authored-by: Bugsounet - Cédric <[email protected]>
Co-authored-by: jkriegshauser <[email protected]>
Co-authored-by: illimarkangur <[email protected]>
Co-authored-by: sam detweiler <[email protected]>
Co-authored-by: vppencilsharpener <[email protected]>
Co-authored-by: veeck <[email protected]>
Co-authored-by: Paranoid93 <[email protected]>
Co-authored-by: Brian O'Connor <[email protected]>
Co-authored-by: WallysWellies <[email protected]>
Co-authored-by: Jason Stieber <[email protected]>
Co-authored-by: jargordon <[email protected]>
Co-authored-by: Daniel <[email protected]>
Co-authored-by: Ryan Williams <[email protected]>
Co-authored-by: Panagiotis Skias <[email protected]>
Co-authored-by: Marc Landis <[email protected]>
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