settings not disabled when group has changed [CPP-782] #897
Conversation
adrian-kong
changed the title
|
Frontend and Release Workflow Started here |
| self.write_setting(group, name, value)?; | ||
| if !in_config { | ||
| Ok(Some(original)) | ||
| Ok(original) // revert to original if not in new config |
There was a problem hiding this comment.
How does this handle the case where there is no original and it's not in the group, does it set the values to the default?
There was a problem hiding this comment.
when theres no original, it wont revert back to original i.e. it gets set
since we can't fetch the field.
this was because ntrip.enable wasn't found - which would turn ntrip to false by default
(this also assumes if its not found, it was false and same with ethernet, if not found its in config mode)
^ not sure if that is true or not however.
EDIT: also added some extra comments,
think it might be best to add tests (but i feel like might be kinda funky 🤔)
There was a problem hiding this comment.
also this method no longer needs to return a result (if we allow this case ^^ - which im not sure if ntrip should be false by default if not found, or ethernet in config by default)
|
@adrian-kong Can you add a note to the PR about how this was tested? |
added 👍 |
| Some(v) => v, | ||
| None => return Ok(true), // update since current values are empty | ||
| }; | ||
| for (name, new_value) in new_group.iter() { |
There was a problem hiding this comment.
this was basically the one of the main issue.
it did not check ntrip.debug as 'old values' did not have this field.
and hence when we tried to toggle ntrip.debug, we didn't actually disable ntrip which would make the first error
| .clone(); | ||
| .get(group, name) | ||
| .map(|x| x.value.clone()) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
this was the second issue, ntrip.enable field was not found in old values and would error here.
this now assumes the groups are off by default so we won't unset it
adrian-kong
changed the title
3 participants
TLDR: previously iterated over existing settings which did not handle case where we attempted to set configs that werent set causing it to not disable ntrip / ethernet before setting.
This was tested by running
modifying ntrip in config.ini file,
previously would result in
caused by iterating over the current settings
(since original did not have debug, it would assume the group was not changed)
the new changes iterate over the modified keys (and checking over the changes instead)
the added logging statements show which settings were toggled and can see now ntrip gets disabled prior to settings.
--writein swift-settings command will need to manually specify this procedure