-
Notifications
You must be signed in to change notification settings - Fork 464
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
Fix organization prior settings #2518
Conversation
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.
mostly minor comments
(metricDefaults.priorSettings ?? { | ||
override: false, | ||
proper: false, | ||
mean: 0, | ||
stddev: DEFAULT_PROPER_PRIOR_STDDEV, | ||
}), |
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.
should the { override: false, ... }
object be defined as a default in shared/constants?
// Metric defaults are nested, so take existing metric defaults only if | ||
// they exist and are not empty | ||
Object.keys(newVal[k]).forEach((kk) => { | ||
newVal[k][kk] = settings?.[k]?.[k] ?? newVal[k][kk]; |
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.
This is accomplishing the same as if you used the spread operator (...
), with settings
spread first. Any reason you wouldn't want to do it that way?
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.
It needs to spread settings?.[k] which could work.
Your preview environment pr-2518-bttf has been deployed. Preview environment endpoints are available at: |
ty, landing in the interest of time, your feedback is useful and i can do it in a follow up. |
Fix bug with setting priors at the org level
Previously,
settings
from the userContext (which pulls from the DB) would be clobbering the sub-fields ofmetricDefaults
that were initialized in the form defaultValues. This isn't a problem for other fields that are not nested, but it is a problem formetricDefaults.priorSettings
which will be missing in most databases.This fixes that issue in the org settings and makes the FE a bit more robust to this issue.
A migration might be a better solution, to be honest.