plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
452 stars 612 forks source link

ObjectBrowserBody treats `data` as Array, when it can be an object #4602

Open tiberiuichim opened 1 year ago

tiberiuichim commented 1 year ago

image

Here we see that this.props.data is an object:

https://github.com/plone/volto/blob/0cca3b257b9fc98da773d3aa8f73a923458a9b59/src/components/manage/Sidebar/ObjectBrowserBody.jsx#L248

But here data is treated as an array, without any checks:

https://github.com/plone/volto/blob/0cca3b257b9fc98da773d3aa8f73a923458a9b59/src/components/manage/Sidebar/ObjectBrowserBody.jsx#LL321C10-L321C10

const x = {};
x.length

<- undefined
tiberiuichim commented 1 year ago

The whole component should be cleaned up, it's hard to understand it as there's too many edge cases and variations of the type of input props.

falgunmpatel commented 1 year ago

@tiberiuichim I think Object.keys(this.props.data).length would give us the length of data object if we actually need it in this code. We can change all three instances of this.props.data.length with Object.keys(this.props.data).length.

falgunmpatel commented 1 year ago

@tiberiuichim I think Object.keys(this.props.data).length would give us the length of data object if we actually need it in this code. We can change all three instances of this.props.data.length with Object.keys(this.props.data).length.

@tiberiuichim Assign this issue to me, if you need me to change it.

tiberiuichim commented 1 year ago

@falgunmpatel it's not only a matter of fixing the code at the surface, because that would only cause issues, unless we address the structural problems of that component.

For example, the old code had cases like:

this.props.data.length < maxSelectionSize

Due to .length being undefined, the component had the right "combination" for the expected behavior. But if we "fix" the code:

Object.keys(this.props.data).length < maxSelectionSize

I expect the component will no longer function properly, because we go into unforseen combination of cases

falgunmpatel commented 1 year ago

@tiberiuichim I apologize for my previous statement. Your example highlights the importance of thoroughly understanding the code and its expected behavior before making any changes. I appreciate your attention to detail and expertise in this matter. Thank you for bringing this to my attention.