Skip to content
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: can retrive COFT with COT automatically #517

Merged
merged 4 commits into from
Jan 4, 2022
Merged

fix: can retrive COFT with COT automatically #517

merged 4 commits into from
Jan 4, 2022

Conversation

WillieRuemmele
Copy link
Member

What does this PR do?

fixes retrieval of COFT

What issues does this PR fix or reference?

forcedotcom/cli#1233, forcedotcom/cli#1241, forcedotcom/cli#1262, @W-10222972@

Functionality Before

retrieve -m CustomFieldTranslation without a CustomObjectTranslation in the org would only pull the CustomFieldTranslation files, which were invalid by themselves without a CustomObjectTranslation parent file

Functionality After

a retrieve will retrieve both COFT/COT and write an empty COT if one doesn't exist in the org

@WillieRuemmele WillieRuemmele requested review from a team as code owners December 22, 2021 23:00
@WillieRuemmele WillieRuemmele requested a review from jag-j December 22, 2021 23:00
CustomObjectFieldTranslations are only addressable through their parent, and require a
CustomObjectTranslation file to be present
*/
if (childType.requiresParent && composedMetadata.length <= 2) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could use the isAddressable field, because COFT is the only metadata type to set either of those. It just seemed like the naming and intention could become confusing if I used isAddressable 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna defer to Steve's opinion on this one.

My question is whether it's valid to have isAddressable:false without requiresParent:true, or vice versa?

Or would you say that what we really meant was unaddressableWithoutParent ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know there are more entities that are unaddressable, and that field is useful and makes sense so that one should remain. I agree with Shane that unaddressableWithoutParent is better. COT and COFT are unfortunate anomalies that hopefully will never happen again. unaddressableWithoutParent is the nicest description of this disastrous pair.

Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd ship it as is if it fixed the problem. I'd like to see @shetzel opine on whether this should be a new property on MetadataType or not.

If it should be a new prop AND it's only valid in combination with isAddressable, then I'd want a validation to be added for that in registryValidation (verify that no childTypes exist with an invalid combination of those properties AND that they can only be used on childTypes).

@@ -61,7 +62,7 @@ export class DecomposedMetadataTransformer extends BaseMetadataTransformer {
const childTypeId = type.children?.directories[tagKey];
if (childTypeId) {
const childType = type.children.types[childTypeId];
const tagValues = Array.isArray(tagValue) ? tagValue : [tagValue];
const tagValues = normalizeToArray(tagValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@@ -25,6 +25,7 @@ export interface MetadataRegistry {
* Metadata type definition in the registry.
*/
export interface MetadataType {
requiresParent?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add the jsdoc for that the option does, like all the others (if we decide to use the new option). Otherwise, modify isAddressable's explanation

CustomObjectFieldTranslations are only addressable through their parent, and require a
CustomObjectTranslation file to be present
*/
if (childType.requiresParent && composedMetadata.length <= 2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna defer to Steve's opinion on this one.

My question is whether it's valid to have isAddressable:false without requiresParent:true, or vice versa?

Or would you say that what we really meant was unaddressableWithoutParent ?

@shetzel shetzel merged commit 7b9a464 into main Jan 4, 2022
@shetzel shetzel deleted the wr/coft branch January 4, 2022 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants