Skip to content

Commit 3335781

Browse files
Remove unnecessary conditionals and fix falsy property check in imperial conversion (#4135)
While removing unnecessary conditionals (for `compliments` reported in https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/27 and for `weather` reported in https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/28), I noticed a bug in the imperial conversion path: falsy property checks like `if (imperialWeatherObject.temperature)` silently skip the conversion when the value is `0`. So at exactly 0 °C, the result would show `0 °F` instead of the correct `32 °F`. I wrote unit tests for `convertWeatherObjectToImperial` first, which confirmed the bug, then fixed it by replacing the falsy checks with the `in` operator. Which I also find more intuitive to read. Weather APIs deliver floating point values, so hitting exactly `0.0` might be rare in practice - more likely `0.1` or `-0.1`, which are truthy and convert fine. That should explain why it went unnoticed by users.
1 parent f2759ad commit 3335781

3 files changed

Lines changed: 54 additions & 9 deletions

File tree

defaultmodules/compliments/compliments.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ Module.register("compliments", {
147147
timeOfDay = "evening";
148148
}
149149

150-
if (timeOfDay && this.config.compliments.hasOwnProperty(timeOfDay)) {
150+
if (this.config.compliments.hasOwnProperty(timeOfDay)) {
151151
compliments = [...this.config.compliments[timeOfDay]];
152152
}
153153

defaultmodules/weather/weatherutils.js

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -187,14 +187,12 @@ const WeatherUtils = {
187187

188188
let imperialWeatherObject = { ...weatherObject };
189189

190-
if (imperialWeatherObject) {
191-
if (imperialWeatherObject.feelsLikeTemp) imperialWeatherObject.feelsLikeTemp = this.convertTemp(imperialWeatherObject.feelsLikeTemp, "imperial");
192-
if (imperialWeatherObject.maxTemperature) imperialWeatherObject.maxTemperature = this.convertTemp(imperialWeatherObject.maxTemperature, "imperial");
193-
if (imperialWeatherObject.minTemperature) imperialWeatherObject.minTemperature = this.convertTemp(imperialWeatherObject.minTemperature, "imperial");
194-
if (imperialWeatherObject.precipitationAmount) imperialWeatherObject.precipitationAmount = this.convertPrecipitationToInch(imperialWeatherObject.precipitationAmount, imperialWeatherObject.precipitationUnits);
195-
if (imperialWeatherObject.temperature) imperialWeatherObject.temperature = this.convertTemp(imperialWeatherObject.temperature, "imperial");
196-
if (imperialWeatherObject.windSpeed) imperialWeatherObject.windSpeed = this.convertWind(imperialWeatherObject.windSpeed, "imperial");
197-
}
190+
if ("feelsLikeTemp" in imperialWeatherObject) imperialWeatherObject.feelsLikeTemp = this.convertTemp(imperialWeatherObject.feelsLikeTemp, "imperial");
191+
if ("maxTemperature" in imperialWeatherObject) imperialWeatherObject.maxTemperature = this.convertTemp(imperialWeatherObject.maxTemperature, "imperial");
192+
if ("minTemperature" in imperialWeatherObject) imperialWeatherObject.minTemperature = this.convertTemp(imperialWeatherObject.minTemperature, "imperial");
193+
if ("precipitationAmount" in imperialWeatherObject) imperialWeatherObject.precipitationAmount = this.convertPrecipitationToInch(imperialWeatherObject.precipitationAmount, imperialWeatherObject.precipitationUnits);
194+
if ("temperature" in imperialWeatherObject) imperialWeatherObject.temperature = this.convertTemp(imperialWeatherObject.temperature, "imperial");
195+
if ("windSpeed" in imperialWeatherObject) imperialWeatherObject.windSpeed = this.convertWind(imperialWeatherObject.windSpeed, "imperial");
198196

199197
return imperialWeatherObject;
200198
}

tests/unit/modules/default/weather/weather_utils_spec.js

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,4 +113,51 @@ describe("Weather utils tests", () => {
113113
}
114114
});
115115
});
116+
117+
describe("convertWeatherObjectToImperial", () => {
118+
it("should return null for null or empty input", () => {
119+
expect(WeatherUtils.convertWeatherObjectToImperial(null)).toBeNull();
120+
expect(WeatherUtils.convertWeatherObjectToImperial({})).toBeNull();
121+
});
122+
123+
it("should convert 0°C correctly to 32°F", () => {
124+
const result = WeatherUtils.convertWeatherObjectToImperial({ temperature: 0 });
125+
expect(result.temperature).toBe(32);
126+
});
127+
128+
it("should convert all temperature fields", () => {
129+
const result = WeatherUtils.convertWeatherObjectToImperial({
130+
temperature: 0,
131+
feelsLikeTemp: 0,
132+
minTemperature: -10,
133+
maxTemperature: 10
134+
});
135+
expect(result.temperature).toBe(32);
136+
expect(result.feelsLikeTemp).toBe(32);
137+
expect(result.minTemperature).toBe(14);
138+
expect(result.maxTemperature).toBe(50);
139+
});
140+
141+
it("should convert windSpeed correctly", () => {
142+
const result = WeatherUtils.convertWeatherObjectToImperial({ windSpeed: 10 });
143+
expect(result.windSpeed).toBeCloseTo(22.369, 2);
144+
});
145+
146+
it("should convert precipitationAmount correctly", () => {
147+
const result = WeatherUtils.convertWeatherObjectToImperial({ precipitationAmount: 25.4, precipitationUnits: "mm" });
148+
expect(result.precipitationAmount).toBeCloseTo(1, 5);
149+
});
150+
151+
it("should not modify properties that are not present", () => {
152+
const result = WeatherUtils.convertWeatherObjectToImperial({ temperature: 20 });
153+
expect("windSpeed" in result).toBe(false);
154+
expect("feelsLikeTemp" in result).toBe(false);
155+
});
156+
157+
it("should not mutate the original object", () => {
158+
const input = { temperature: 20 };
159+
WeatherUtils.convertWeatherObjectToImperial(input);
160+
expect(input.temperature).toBe(20);
161+
});
162+
});
116163
});

0 commit comments

Comments
 (0)