feat(backup-ng/new): group saved & new schedules #3093
Conversation
badrAZ
changed the title
| @@ -47,14 +46,21 @@ import { FormFeedback, FormGroup, Input, Number, Ul, Li } from './utils' | |||
|
|
|||
| const normaliseTagValues = values => resolveIds(values).map(value => [value]) | |||
|
|
|||
| const normaliseCopyRentention = settings => { | |||
| const normaliseCopyRetention = settings => { | |||
| ...getNewSettings({ | ||
| schedules: state.newSchedules, | ||
| ...normaliseSettings({ | ||
| settings: cloneDeep(state.settings), |
There was a problem hiding this comment.
Why do you need to clone this?
There was a problem hiding this comment.
Because the function normalizeSettings mutate the settings' properties in deep.
| @@ -202,10 +178,10 @@ export default [ | |||
| name: state.name, | |||
| mode: state.isDelta ? 'delta' : 'full', | |||
| compression: state.compression ? 'native' : '', | |||
| schedules: getNewSchedules(state.newSchedules), | |||
| schedules: normalizeSchedules(cloneDeep(state.schedules)), | |||
There was a problem hiding this comment.
IMHO, this would be simpler:
schedules: mapValues(state.schedules, ({ id, ...schedule }) => schedule)
|
|
||
| if (newSchedule === undefined) { | ||
| return deleteSchedule(scheduleId) | ||
| return deleteSchedule(id) |
There was a problem hiding this comment.
Is settings correctly updated regarding this deletion?
There was a problem hiding this comment.
yes, the effect deleteSchedule update the schedules and settings vars.
| ) { | ||
| return editSchedule({ | ||
| id, | ||
| jobId: props.job.id, |
There was a problem hiding this comment.
Why do you need to send this property?
Can it change?
There was a problem hiding this comment.
My bad, i will fix it.
| }) | ||
| if ( | ||
| newSchedule.cron !== oldSchedule.cron || | ||
| newSchedule.timezone !== oldSchedule.timezone |
There was a problem hiding this comment.
newSchedule should simply contains the values that has been edited by the user.
| const newSetting = settings[id] | ||
| const settings = cloneDeep(state.settings) | ||
| await Promise.all([ | ||
| ...map(state.schedules, async schedule => { |
There was a problem hiding this comment.
It's an error, i will fix it.
| for (const id in oldSettings) { | ||
| const oldSetting = oldSettings[id] | ||
| const newSetting = settings[id] | ||
| const settings = cloneDeep(state.settings) |
There was a problem hiding this comment.
Do you really need cloneDeep?
Isn't { ...state.settings } enough?
There was a problem hiding this comment.
Nope, my bad, due to the normalizeSettings below.
See #2711
Check list
Fixes #007)Process
WiP:(Work in Progress) if not ready to be mergedList of packages to release