-
Notifications
You must be signed in to change notification settings - Fork 27
Updated fetchValue() function to handle repo level configuration fall… #429
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
base: main
Are you sure you want to change the base?
Conversation
…back to org level configuration as fetched
blocks/edit/da-assets/da-assets.js
Outdated
const data = null; | ||
|
||
// Start with organization-level config | ||
if (orgConf) { | ||
data = {}; | ||
orgConf.forEach((item) => { | ||
data[item.key] = item.value; | ||
}); | ||
} | ||
|
||
// Override with repository-level config | ||
if (repoConf) { | ||
if (data === null) data = {}; | ||
repoConf.forEach((item) => { | ||
data[item.key] = item.value; | ||
}); | ||
} | ||
|
||
if (!data) return null; | ||
|
||
const confKey = data.find((conf) => conf.key === key); |
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.
data
was an array previously, and is now an object which means that the data.find
will fail on line 60. Now that it's an object, you can just do a lookup
const data = null; | |
// Start with organization-level config | |
if (orgConf) { | |
data = {}; | |
orgConf.forEach((item) => { | |
data[item.key] = item.value; | |
}); | |
} | |
// Override with repository-level config | |
if (repoConf) { | |
if (data === null) data = {}; | |
repoConf.forEach((item) => { | |
data[item.key] = item.value; | |
}); | |
} | |
if (!data) return null; | |
const confKey = data.find((conf) => conf.key === key); | |
const data = {}; | |
// Start with organization-level config | |
orgConf?.forEach((item) => { | |
data[item.key] = item.value; | |
}); | |
// Override with repository-level config | |
repoConf?.forEach((item) => { | |
data[item.key] = item.value; | |
}); | |
return data[key]; |
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 will be updating the PR to treat data as an array to retain the expected data type of data
variable. Pushing objects to an array and updating values inside them if the keys match afterwards, is my approach.
|
||
// Determine if images should be links | ||
const injectLink = (await getConfKey(owner, repo, 'aem.assets.image.type')) === 'link'; | ||
const injectLink = |
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.
This change and below don't need to be refactored
Hi @chrischrischris , I didn't really understand as to why the path variable is required to be passed anyways. Since configuration merge is fetched from an exhaustive list of paths. I decided to check whether paths are matching with the expected org and repo level paths, fetch the configurations and merge as required. All other fetchValue() calls which dont match that path patterns will still fall through and return the value fetched as expected in the original function body. |
…back to org level configuration as fetched
Description
I have updated the fetchValue() function body to fetch both the organization level and repo level configurations which are then merged and returned on call. Repo level configurations as found will be considered as final over organization level configurations.
Related Issue
#422
Motivation and Context
As of right now, the org fallback only works if you have no site config. We should inherit on a row basis so that if a site has a single property (aem.assets.link.type) the rest still falls back to org so you don't have to duplicate your repoId, etc.
To solve this as mention in issue #422.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: