Cleanup compliments module

[rejas]

Lots of small fixes and cleanups:

• only render something when there is a compliment
• cleanup naming
• use es6 notations
• use fetch instead of XMLHttpRequest in compliments

[rejas]

@khassel asking you as the test-specialist ;-) : how can I fix the tests not finding "fetch" when I switch the module xmlhttprequest to fetch?

  ● Compliments module › remoteFile option › should show compliments from a remote file

    ReferenceError: fetch is not defined

[khassel]

will try to look at this but I have very limited time until end of this month ...

[ghost]

This was one of the simplest modules that worked properly, now is not working with HTML compliments like:

	"day_sunny": [
		"<i class=\"gold wi wi-day-sunny\"></i> Sunny day",
	],

[rejas]

This was one of the simplest modules that worked properly, now is not working with HTML compliments like:

	"day_sunny": [
		"<i class=\"gold wi wi-day-sunny\"></i> Sunny day",
	],

when did it stop working? with this PR?

[ghost]

This was one of the simplest modules that worked properly, now is not working with HTML compliments like:

	"day_sunny": [
		"<i class=\"gold wi wi-day-sunny\"></i> Sunny day",
	],

when did it stop working? with this PR?

yes

[sdetweil]

not sure, but this moved the span inside the newline loop. so u will have a span per part.. instead of one.

and if split didn't, then you would lose the span

[rejas]

This was one of the simplest modules that worked properly, now is not working with HTML compliments like:

	"day_sunny": [
		"<i class=\"gold wi wi-day-sunny\"></i> Sunny day",
	],

when did it stop working? with this PR?

yes

This doesnt work for me neithe ron the develop branch nor on the master branch. What version of MM2 are you using and how does your config look like in total (minus your secrets)?

[khassel]

how can I fix the tests not finding "fetch" when I switch the module xmlhttprequest to fetch?

I don't know. This is the first time someone is using the browser fetch function.

The e2e tests are running with jsdom and it seems that fetch is not implemented there, see jsdom/jsdom#1724. There is a workaround, may you are able to get this working (but I'm not sure if it is worth to get another devDependency for one test).

[khassel]

adding the line dom.window.fetch = corefetch; in the following function of tests/e2e/helpers/global-setup.js seems to work:

exports.getDocument = () => {
        return new Promise((resolve) => {
                const url = "http://" + (config.address || "localhost") + ":" + (config.port || "8080");
                jsdom.JSDOM.fromURL(url, { resources: "usable", runScripts: "dangerously" }).then((dom) => {
                        dom.window.name = "jsdom";
                        dom.window.fetch = corefetch;
                        dom.window.onload = () => {
                                global.document = dom.window.document;
                                resolve();
                        };
                });
        });
};

[codecov-commenter]

Codecov Report

Merging #2965 (4a93f06) into develop (eee289a) will increase coverage by 0.09%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #2965      +/-   ##
===========================================
+ Coverage    22.75%   22.85%   +0.09%     
===========================================
  Files           51       51              
  Lines        10895    10887       -8     
===========================================
+ Hits          2479     2488       +9     
+ Misses        8416     8399      -17     

Impacted Files	Coverage Δ
modules/default/compliments/compliments.js	0.00% <0.00%> (ø)
modules/default/updatenotification/node_helper.js	80.00% <0.00%> (+12.85%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[rejas]

adding the line dom.window.fetch = corefetch; in the following function of tests/e2e/helpers/global-setup.js seems to work: ```

That did indeed help, thx!

[khassel]

I'm fine with this but I don't know if the other commenters are ...

[rejas]

I'm fine with this but I don't know if the other commenters are ...

Cleaned up after @sdetweil's comment so that only one span is now generated again.

As for @Badatheist I cannot reproduce that. Doesnt work on develop nor on the last master release. Without more information I cannot do anything here.

[ghost]

As for @Badatheist I cannot reproduce that. Doesnt work on develop nor on the last master release. Without more information I cannot do anything here.

What more information? It's very simple, before the compliments worked with HTML format, after the change they don't work anymore. I will stick to the old version which works perfectly for me. Not every update is good if it is not properly thought out and tested, it is valid for any software. For me it's a bug. Good luck and stay healthy!

[rejas]

@Badatheist sorry to hear that, but like I wrote: I couldnt get your config entry to work, neither with my changes of course, but neither with the develop branch nor with the last release.

Without more information from your side (for example what version are you currently running, what is your whole config) I cannot help you out and get this bug fixed.