-
Notifications
You must be signed in to change notification settings - Fork 70
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
Bug fix in update selector func #152
base: master
Are you sure you want to change the base?
Changes from all commits
f0c7f3d
7b6412e
a05cdda
3e966a3
638f8e3
4714964
0351045
d006ed2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -204,6 +204,12 @@ export default class SitemapController { | |
'.delete_selector_submit': { | ||
click: this.confirmDeleteSelector, | ||
}, | ||
'.update_selector_submit': { | ||
click: this.confirmDeleteChildSelectors, | ||
}, | ||
'#modal-save-child-selectors': { | ||
click: this.confirmSaveChildSelectors, | ||
}, | ||
'.delete_sitemap_submit': { | ||
click: this.confirmDeleteSitemap, | ||
}, | ||
|
@@ -1401,7 +1407,38 @@ export default class SitemapController { | |
const newSelector = this.getCurrentlyEditedSelector(); | ||
const validator = this.getFormValidator(); | ||
validator.revalidateField('id'); | ||
// cancel submit if invalid form | ||
if (selector.type !== newSelector.type) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Modal init should be after form validation and removeCurrentContentSelector call, otherwise it is possible to create invalid selector |
||
const children = sitemap.selectors.filter(selectorFromList => | ||
selectorFromList.parentSelectors.includes(selector.uuid) | ||
); | ||
if (children.length > 0) { | ||
this.initConfirmActionPanel({ action: 'update_selector' }); | ||
if (newSelector.canHaveChildSelectors()) { | ||
const saveSelectorsBtn = $('<button/>', { | ||
class: 'btn btn-primary', | ||
id: 'modal-save-child-selectors', | ||
text: Translator.getTranslationByKey( | ||
'modal_confirm_action_submit_update_save_selector' | ||
), | ||
}); | ||
$('.modal-footer').append(saveSelectorsBtn); | ||
$('#modal-title').text( | ||
Translator.getTranslationByKey( | ||
'modal_confirm_action_title_update_selector_can_have_child' | ||
) | ||
); | ||
} | ||
$('#modal-child-count').text(children.length); | ||
$('#modal-message').after('<ul id="list-deleted-children"></ul>'); | ||
children.forEach(child => { | ||
const $child = $('<li></li>'); | ||
$child.text(`#${child.uuid} ${child.id}`); | ||
$('#list-deleted-children').append($child); | ||
}); | ||
$('#modal-message').show(); | ||
Comment on lines
+1431
to
+1438
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extract duplicated code into method |
||
return true; | ||
} | ||
} | ||
if (!this.isValidForm()) { | ||
return false; | ||
} | ||
|
@@ -1411,16 +1448,29 @@ export default class SitemapController { | |
} catch (err) { | ||
console.error(err); | ||
} | ||
await this.updateSelector(); | ||
} | ||
async updateSelector(saveChildSelectors = false) { | ||
Comment on lines
+1452
to
+1453
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add newlines between methods |
||
const sitemap = this.state.currentSitemap; | ||
const selector = this.state.currentSelector; | ||
const newSelector = this.getCurrentlyEditedSelector(); | ||
try { | ||
sitemap.updateSelector(selector, newSelector); | ||
sitemap.updateSelector(selector, newSelector, saveChildSelectors); | ||
await this.store.saveSitemap(sitemap, undefined, this.getCurrentProjectId()); | ||
} catch (err) { | ||
console.error(err); | ||
} finally { | ||
this.showSitemapSelectorList(); | ||
} | ||
} | ||
|
||
async confirmSaveChildSelectors(button) { | ||
await this.updateSelector(true); | ||
$('#confirm-action-modal').modal('hide'); | ||
} | ||
async confirmDeleteChildSelectors(button) { | ||
await this.updateSelector(false); | ||
$('#confirm-action-modal').modal('hide'); | ||
} | ||
/** | ||
* Get selector from selector editing form | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,13 +152,32 @@ export default class Sitemap { | |
return urls; | ||
} | ||
|
||
updateSelector(selector, selectorData) { | ||
updateSelector(selector, selectorData, saveChildSelectors = false) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like that the signature suggests children may be deleted even if selector type is not changed. We can think of another naming here (e.g. saveChildrenForNewType?), or implement deliberate removal - maybe it will simplify the code. |
||
// selector is undefined when creating a new one and delete old one, if it exist | ||
if (selector === undefined || selector.type !== selectorData.type) { | ||
if (selector) { | ||
this.deleteSelector(selector); | ||
if ( | ||
selector.canHaveChildSelectors() && | ||
selectorData.canHaveChildSelectors() && | ||
saveChildSelectors | ||
) { | ||
//custom logic: we don’t delete children, but redefined them a parent | ||
const children = this.selectors.filter(selectorFromList => | ||
selectorFromList.parentSelectors.includes(selector.uuid) | ||
); | ||
const newSelector = SelectorList.createSelector(selectorData); | ||
children.forEach(child => { | ||
const parentUuidIndex = child.parentSelectors.indexOf(selector.uuid); | ||
child.parentSelectors[parentUuidIndex] = newSelector.uuid; | ||
}); | ||
selector = newSelector; | ||
Comment on lines
+164
to
+173
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar logic is present below in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can try to simplify this, after the main part of the logic is approved. |
||
} else { | ||
this.deleteSelector(selector); | ||
selector = SelectorList.createSelector(selectorData); | ||
} | ||
} else { | ||
selector = SelectorList.createSelector(selectorData); | ||
} | ||
selector = SelectorList.createSelector(selectorData); | ||
} | ||
|
||
// update child selectors | ||
|
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.
why "additional"?