-
Notifications
You must be signed in to change notification settings - Fork 14
[OF-1733] feat!: Read default settings from login #803
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
[OF-1733] feat!: Read default settings from login #803
Conversation
🚀 Pull Request Review GuidelinesThank you for taking the time to review this Pull Request. The following is a summary of our Pull Request guidelines. The full guidelines can be found here. 💬 How to Provide FeedbackWe use a comment ladder when leaving review comments to avoid any ambiguity.
All comments should be prefixed with one of the above tags, for example: 🎯 PR Author Focus Areas
Thanks again for taking the time to review this Pull Request. |
8b8305d to
80b4bd1
Compare
Library/include/CSP/Systems/Settings/ApplicationSettingsSystem.h
Outdated
Show resolved
Hide resolved
Library/include/CSP/Systems/Settings/ApplicationSettingsSystem.h
Outdated
Show resolved
Hide resolved
Library/include/CSP/Systems/Settings/ApplicationSettingsSystem.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the changes look good, @MAG-AdamThorn raise a good point around result objects that I have commented on, will leave this unapproved until thread are resolved.
Library/include/CSP/Systems/Settings/ApplicationSettingsSystem.h
Outdated
Show resolved
Hide resolved
|
@MAG-AdamThorn @MAG-ChristopherAtkinson We can't move the Result objects themselves to |
Need to do this because we need to put them on the login response, which is common. It's a bit awkward, as we also have the result types which are `systems`. I'm thinking that result types should begin to be centralised into their own namespace. As `ApplicationSettings` only had the result type stuff left in the file, I deleted it and put it into `ApplicationSettingsSystem` ... although honestly i'm not sure about this. It's non-symmetric with the already non-symettrically named `SettingsCollection`
If the server returns default settings, put them in the login response.
Remove sarcastic comment
Just an organizational choice.
Hopefully temporary skip that will be removed shortly when localMCS is seeding correctly.
b171b72 to
d71caf3
Compare

Read the default settings now returned from login, and provide them in the LoginState return.
As LoginState is common, had to move existing settings structures to common also, which is what makes this a breaking change.
This entailed the removal/splitting up of the
ApplicationSettings.hfile, I've just put the stuff that remained from there inApplicationSettingsSystemBreaking Changes
csp::systems::SettingsCollectionrelocated tocsp::common::SettingsCollection.csp::systems::ApplicationSettingsrelocated tocsp::common::ApplicationSettings.New Api
Returned object from Login,
LoginStatenow contains:csp::common::List<csp::common::ApplicationSettings> DefaultApplicationSettingscsp::common::List<csp::common::SettingsCollection> DefaultSettings;Important
The test will fail until CHS have resolved their issue with seeding Local MCS. Internal slack thread here. It passes against live MCS.
This change is rather non-symmetric. Mapping UserSettings to SettingsCollection but not mirroring the same with ApplicationSettings makes it rather tricky to reason about what setting is relevent to the
SettingsSystemand what isn't, both from an api consumer perspective as well as a library developer trying to maintain module boundaries perspective. You'll see some of that in this PR.