Add weather display settings and ExcludedPeople filter #582
Add weather display settings and ExcludedPeople filter #582nopoz wants to merge 8 commits intoimmichFrame:mainfrom
Conversation
… int (0-2) for flexible temperature precision
📝 WalkthroughWalkthroughAdds per-account person exclusions to asset loading and introduces configurable temperature display settings (unit visibility and decimal precision) across configs, models, adapters, core pool logic, tests, and frontend. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ImmichFrame.WebApi.Tests/Helpers/Config/ConfigLoaderTest.cs`:
- Around line 123-131: The assertion for "TemperatureDecimalDigits" in
ConfigLoaderTest currently expects 7 which is outside the allowed 0–2 range;
update the expectation in the if/else block that checks prop.Name ==
"TemperatureDecimalDigits" (in the ConfigLoaderTest test) to a valid value
(e.g., expect 1 for V1 default and 2 for the V2 fixture) and ensure the test
fixture that supplies value aligns with that allowed value; adjust the branch
using expectNullApiKeyFile and the Assert.That(value, Is.EqualTo(...),
prop.Name) accordingly so the asserted value is within 0–2.
In `@ImmichFrame.WebApi.Tests/Resources/TestV2.json`:
- Around line 33-34: The fixture TestV2.json contains "TemperatureDecimalDigits"
set to 7 which is outside the validated 0–2 range; update
"TemperatureDecimalDigits" to a valid value (e.g., 0, 1, or 2 — commonly 2) in
the TestV2.json fixture and then adjust any tests that assert on
formatting/precision to expect the new valid digit count; ensure the keys
"ShowTemperatureUnit" and "TemperatureDecimalDigits" are left intact and only
the numeric value is changed so tests reflect valid production configuration.
🧹 Nitpick comments (3)
docker/Settings.example.yml (1)
30-31: Missing new temperature display settings in example configuration.The PR introduces
ShowTemperatureUnitandTemperatureDecimalDigitssettings, but they are not included in this example file. For completeness and user discoverability, consider adding them to the General section:ShowWeatherDescription: true ShowTemperatureUnit: true TemperatureDecimalDigits: 1ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (1)
33-43: Add tests for ExcludedPeople filtering behavior.The implementation actively uses the
ExcludedPeoplesetting to filter assets (loads excluded person assets and removes them from results), but the current tests never exercise this logic. Consider adding test cases that verify:
- Assets from excluded people are filtered out
- Exclusion works correctly with multiple excluded people
- Behavior when a person appears in both
PeopleandExcludedPeopleImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
11-24: Skip excluded-people fetch when no People filter is configured.When
Peopleis null/empty, the method returns an empty result anyway, so excluded-person fetches become unnecessary API calls.♻️ Suggested refactor
- var excludedPersonAssets = new List<AssetResponseDto>(); - - foreach (var personId in accountSettings.ExcludedPeople) - { - excludedPersonAssets.AddRange(await LoadAssetsForPerson(personId, ct)); - } - var personAssets = new List<AssetResponseDto>(); var people = accountSettings.People; - if (people == null) + if (people == null || people.Count == 0) { return personAssets; } + + var excludedPersonAssets = new List<AssetResponseDto>(); + foreach (var personId in accountSettings.ExcludedPeople) + { + excludedPersonAssets.AddRange(await LoadAssetsForPerson(personId, ct)); + }
- Move People null check before ExcludedPeople loading in PeopleAssetsPool - to avoid unnecessary API calls, and fix TemperatureDecimalDigits test - fixture value from 7 to 2 to match the validated 0-2 range.
|
I'm still good with the weather changes, but did have a thought. Why are we showing weather units at all/ever? Isn't it understood that the units are going to be in whatever locale you are in? I'm just thinking cars don't show this, signs on the roadway don't show this, my MacOS widget I'm looking at right now doesn't have units. In other words maybe we just remove this and then this extra setting is unneeded? I still have concerns with the ExcludedPeople, since this will not work when people use in conjunction with Albums. This seems like it will cause confusion, even though it's not our fault. |
JW-CH
left a comment
JW-CH
left a comment
There was a problem hiding this comment.
Hey, could you split this into separate PRs? It would make reviewing a lot easier when each PR focuses on one thing.
They are in separate PRs. Please read the description. 😕 |
I agree, but @JW-CH was making an argument in the original PR to keep it, although never clarified why exactly. |
|
@nopoz I understand the merge conflict situation, but combining three features into one PR makes review much harder.
My request: |
@JW-CH The separate PRs already exist (#549, #553, #561) — they just have rebase conflicts. This combined PR is those PRs, rebased onto current main with the conflicts resolved. Splitting them back out would mean re-doing that rebase work three times, and they'd immediately conflict with each other again since they touch the same files (IServerSettings.cs, ServerSettings.cs, ClientSettingsDto.cs). Merging them sequentially would require someone to resolve the same conflicts repeatedly. I understand the preference for single-feature PRs, but in this case the features share interface and model changes that make them genuinely interdependent from a merge perspective. The discussion can still happen per-feature — you and @3rob3 are already doing that effectively here. Any code changes that come out of those discussions can be made directly in this PR. Would it help if I added clearer commit separation or section headers in the PR description to make review easier? |
3 participants
Summary
This PR combines three related feature branches that were previously submitted as separate PRs (#549, #553, #561). Those PRs all modified the same core settings files (IServerSettings.cs, ServerSettings.cs, ServerSettingsV1.cs, ClientSettingsDto.cs, test resources, and example
configs), causing persistent merge conflicts between them and with main. Rebasing them individually would have required resolving the same conflicts repeatedly in a chain, so this PR consolidates all changes into a single cleanly-rebased branch.
Included Features
ShowTemperatureUnitsetting (previously Add ShowTemperatureUnit setting to hide F/C from weather display #553)ShowTemperatureUnitboolean property (default: true) across backend and frontendTemperatureDecimalDigitssetting (previously Add setting to adjust temperature number display style #549)TemperatureDecimalDigitsinteger property (default: 1, range: 0-2) for flexible temperature precisionUseWholeNumberTemperaturesboolean with a more flexible approachExcludedPeoplesetting (previously Add "ExcludedPeople" setting to filter out specific people from "People" results #561)ExcludedPeoplelist (List) to account settings, mirroring the existing ExcludedAlbums patternPrevious PRs
This PR supersedes the following, which can be closed once this is merged:
Test Plan
Summary by CodeRabbit
New Features
Tests
Documentation