IBX-10168: Fixed missing assets icons #1619
Conversation
ezrobot
requested review from
Steveb-p,
ViniTou,
adamwojs,
alongosz,
barw4,
ciastektk,
konradoboza,
mikadamczyk,
tbialcz and
wiewiurdp
and removed request for
a team
There was a problem hiding this comment.
Basically what happens is that configuration parser does not care if configuration value is null, empty array ([]), or is actually filled up with some data.
Since parameter defaulted to empty array, it was not possible to go through SiteAccessConfigResolver (site access specific) and defaults were never loaded.
That said, we might need to revisit this, because usually when looking for icon we are looking for a specific one. And if site access does not define a copy, it will not go and try to resolve icon from defaults.
alongosz
changed the title
| @@ -1,5 +1,5 @@ | |||
| system: | |||
| admin_group: | |||
| default: | |||
There was a problem hiding this comment.
Could you please explain reasoning behind this change ?
There was a problem hiding this comment.
In this particular case there were missing icons in customer portal, which isn't one of admin_group, therefore icon mapping was missing there.
There will be same problem with each site access that isn't part of admin_group that was using our icons through icon helper, so default will catch all of them.
There was a problem hiding this comment.
As per description of the issue in the linked ticket:
Missing icon for:
Embed block in PB
Several blocks in Dashboard builder (see screenshot)
Spilt/Unified option in version compare
Customer portal - Members list
Customer portal - Address book - Add
Customer portal -My profile - Edit - AI icon
Customer portal - Invite members modal
This means that there are several site accesses that are missing icon sets. Making those sets part of the default, paired with change that empty icon sets are not considered "valid" for the purposes of establishing a site-access parameter, alleviates the immediate issue.
I would like to open this discussion further into how we could deal with - basically - merging of different configuration levels, which is currently impossible. Site access level of icon sets overwrites site group access settings, which overwrite defaults (don't gonna even mention global...).
But that is a different story and broader in scope than resolving this issue.
|
|
It works!
|
9 participants
Related PRs:
Description:
There's bigger issue in resolving config, also in other bundles, so someone from backend will need to look into it. Without this check site access resolver was always returning empty array therefore it didn't resolved default one. @Steveb-p looked into it, so he'll be able to tell more.
For QA:
Documentation: