-
Notifications
You must be signed in to change notification settings - Fork 420
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
Introduces Metadata realtionships #78
Conversation
Also fixes a slew of minor bugs and pmd issues like: @returns -> @return missing @description etc.
@pozil @msrivastav13 - this is a bigish pr, but most of these are minor edits. I chose to do it here, because no matter what branch i did these fixes on there'd be a ton-of-em |
if (!CanTheUser.read(new Bucketed_Picklist__mdt())) { | ||
return new List<Bucketed_Picklist__mdt>(); | ||
} | ||
String obj = String.valueof(objId.getSObjectType()); |
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.
possible null pointer exception? Not sure if you would like to add a null check here if Id is null.
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.
good catch, addressing.
@@ -0,0 +1,266 @@ | |||
<?xml version="1.0" encoding="UTF-8" ?> |
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 am not sure if adding or modifying the standard layouts is a good idea! If someone installs apex-recipes in production as an unlocked package (or deploying it), there will be overrides.
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 agree with @msrivastav13, I don't recommend it. If I recall well, there was some of that in the original Dreamhouse and it ended "badly".
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.
yeah, I'm removing the layout it was an oversight and shouldn't have been added.
force-app/main/default/layouts/Contact-Contact %28Sales%29 Layout.layout-meta.xml
Outdated
Show resolved
Hide resolved
force-app/main/default/layouts/Contact-Contact %28Support%29 Layout.layout-meta.xml
Outdated
Show resolved
Hide resolved
force-app/main/default/layouts/Contact-Contact Layout.layout-meta.xml
Outdated
Show resolved
Hide resolved
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.
@codefriar This looks good! Except for a couple that we need to probably discuss
- I thought we need to do null handling for public methods. I will leave that to you here to make the decision.
- I am not so comfortable adding a standard layout to the app. It can override if some learning it and installs it. Maybe we can create our custom layout and add the custom field?
Good catch, both. Will address.
…On Jan 26, 2021, 18:57 -0500, Mohith Shrivastava ***@***.***>, wrote:
@msrivastav13 commented on this pull request.
@codefriar This looks good! Except for a couple that we need to probably discuss
1. I thought we need to do null handling for public methods. I will leave that to you here to make the decision.
2. I am not so comfortable adding a standard layout to the app. It can override if some learning it and installs it. Maybe we can create our custom layout and add the custom field?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
force-app/main/default/classes/Custom Metadata Recipes/CustomMetadataUtilties.cls
Show resolved
Hide resolved
force-app/tests/Custom Metadata Recipes/CustomMetadataRecipes_Tests.cls
Outdated
Show resolved
Hide resolved
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.
That took some time to review but it looks good to me. I just left a couple of minor comments, no blockers.
Next time, please move the Jest test updates in a dedicated PR.
Also fixes a slew of minor bugs and pmd issues like:
@returns -> @return
missing @description etc.