-
Notifications
You must be signed in to change notification settings - Fork 163
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 bulk descendants boolean maps #4751
Fix bulk descendants boolean maps #4751
Conversation
...contentNodeData, | ||
...getMergedMapFields(node, contentNodeData), | ||
}; | ||
console.log('contentNodeData', contentNodeData); |
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.
👀
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.
Whoops. Just pushed the changes! Thank you!
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.
The changes look good to me and enhance the initial implementation. I’ve also reviewed the bulk edit feature and haven’t observed any regressions. I'll keep the approval open in case any of the other assignees have additional comments or suggestions.
Thanks @AlexVelezLl
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.
Updating the descendants of a folder works as expected and I see nothing wrong with the code changes!
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.
I've not tested, but code changes make sense to me - and looks like we are just reusing the merge map fields everywhere now, so that all seems consistent.
Thanks @AlexVelezLl - no issues observed while manually testing. |
Summary
Description of the change(s) you made
This PR changes the behaviour when we update boolean map fields when updating all descendants of a folder. This will now just add all new options in boolean maps to descendants instead of replacing them with the current ones.
This PR also fixes a bug that removed all current categories and just added new ones when we inherited suggested categories from the parent folder.
Manual verification steps performed
Screenshots (if applicable)
Compartir.pantalla.-.2024-09-25.09_29_33.mp4
Does this introduce any tech-debt items?
If in the future we want to allow user to choose between add options or replace them, we will probably need to come to this PR and see the changes, as I havent left this extensible for that behaviour.
Reviewer guidance
How can a reviewer test these changes?
Play with the descendants editing feature, for both mapFields (categories, levels, resources needed) and non mapFields (language), and check that it follows the new required behaviour and no regressions are observed.
Are there any risky areas that deserve extra testing?
The whole descendants editing behaviour, whether it is from boolean maps, or normal fields, if its done in another browser and synced with the curreent one, stored changes have not being synced.
References
Closes #4748.
Comments
Contributor's Checklist
PR process:
CHANGELOG
label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocs
label has been added if this introduces a change that needs to be updated in the user docs?requirements.txt
files also included in this PRStudio-specifc:
notranslate
class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages
,components
, andlayouts
directories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarn
andpip
)