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

Import support for custom metadata #238

Merged

Conversation

toly11
Copy link
Contributor

@toly11 toly11 commented Nov 30, 2023

Overview:
This pull request significantly enhances Inspector's capabilities by introducing support for importing and deleting records of custom metadata types. Previously, Inspector's functionality was primarily confined to interacting with the standard Enterprise API and the Tooling API. This extension did not encompass the Metadata API, which limited its utility for custom metadata operations. The implementation of this feature addresses this gap, expanding Inspector's versatility.

Testing
Extensive testing has been conducted across various environments to ensure this new feature's compatibility and seamless integration with Inspector's existing functionalities. I encourage further testing to validate its efficiency and usability enhancements. Furthermore, I have executed all unit tests in accordance with the guidelines outlined in the contribution guide, making minor modifications to the test script to accommodate the new changes

Documentation
I have documented this change in the CHANGES.md file

Request for Review
I kindly request a review of this pull request and am open to making any necessary adjustments based on your feedback. Thank you for considering this valuable enhancement to the extension's functionality

image

@tprouvot
Copy link
Owner

Wow @toly11 🤯
This is a really good one 👏
I'll take some time to perform some tests but it looks good !

@tprouvot
Copy link
Owner

I did some tests and it works well 👍
I would add some automation to save some clicks, for example when pasting a custom metadata object (ending with __mdt) I would automatically select "Metadata" api.

I would also add the same checks that we have for update records when no id is provided (with MasterLabel field for CustomMetadata)
image

Anyway those are small updates, you did already a good job 🙇‍♂️ 🙏

@tprouvot
Copy link
Owner

I'm wondering if we need to display API Type to users, maybe we can just leave it as it was and automatically select metadata when the sobject ends with __mdt ?
What do you think ?

{ value: "create", label: "Insert", supportedApis: ["Enterprise", "Tooling"] },
{ value: "update", label: "Update", supportedApis: ["Enterprise", "Tooling"] },
{ value: "upsert", label: "Upsert", supportedApis: ["Enterprise", "Tooling"] },
{ value: "delete", label: "Delete", supportedApis: ["Enterprise", "Tooling"] },
Copy link
Owner

Choose a reason for hiding this comment

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

You can also add the new option "undelete" I've added yesterday

#237

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I think about this again, Isn't undelete supported in Enterprise API only?
If so, I will remove Tooling from the supportedApis attribute

@toly11
Copy link
Contributor Author

toly11 commented Dec 1, 2023

I did some tests and it works well 👍 I would add some automation to save some clicks, for example when pasting a custom metadata object (ending with __mdt) I would automatically select "Metadata" api.

I would also add the same checks that we have for update records when no id is provided (with MasterLabel field for CustomMetadata) image

Anyway those are small updates, you did already a good job 🙇‍♂️ 🙏

Absolutely, great points! I'll tweak the API type to auto-select Metadata for anything ending in __mdt - that should save a few clicks.
Regarding you'r second point, the MasterLabel field isn't an indicator for the user's intension since it isn't required for either Upsert (when updating) or Delete. Only the DeveloperName is always required. So I think pre-selecting Upsert Metadata is our best option.

I'll commit an update soon.

Edit: updated.

@tprouvot
Copy link
Owner

tprouvot commented Dec 1, 2023

Nice updates 👍
I don't know if you've seen my last comment on the Api select input :

"I'm wondering if we need to display API Type to users, maybe we can just leave it as it was and automatically select metadata when the sobject ends with __mdt ?"
Since the selection is automatic now, we can keep the previous UI, what do you think ?

Have you tried to update CMDT without MasterLabel ?
I have an error when the field is not used

image

@toly11
Copy link
Contributor Author

toly11 commented Dec 1, 2023

I'm wondering if we need to display API Type to users, maybe we can just leave it as it was and automatically select metadata when the sobject ends with __mdt ? What do you think ?

You're onto something with the idea of auto-selecting Metadata API for __mdt objects behind the scenes, I initially went down that path, as seen in my first commit. However, I realized separating API types makes development and future updates smoother. Here's why:

Action Limitations: With Metadata, we only have upsert and delete options. This differs from the other APIs.

Primary Key Differences: For custom metadata, "DeveloperName" is always the key, unlike "Id" or external IDs in other cases. This simplifies the code for the field mapping logic.

Future Feature Integration: Planning ahead, we might want to show the Metadata option only if users have the required permissions (like 'Customize Application').

Support for additional APIs In the future we might want to add additional APIs, this will be easier with this architecture.

Terminology: Also, we've moved beyond a simple Tooling vs. non-Tooling binary. Now, with Enterprise, Tooling, and Metadata as distinct APIs, it's more accurate and clearer to maintain this division in terms.

This approach might complicate the UI slightly, but it's a trade-off for clearer, more maintainable code and functionality.
Your thoughts?

@toly11
Copy link
Contributor Author

toly11 commented Dec 1, 2023

I don't know if you've seen my last comment on the Api select input :

Yes, please see my comment on that message

@toly11
Copy link
Contributor Author

toly11 commented Dec 1, 2023

Nice updates 👍 I don't know if you've seen my last comment on the Api select input :

"I'm wondering if we need to display API Type to users, maybe we can just leave it as it was and automatically select metadata when the sobject ends with __mdt ?" Since the selection is automatic now, we can keep the previous UI, what do you think ?

Have you tried to update CMDT without MasterLabel ? I have an error when the field is not used

image

Have you tried to update CMDT without MasterLabel ?
I have an error when the field is not used

I double checked, and you're right, it is required in upsert operations also on update. So I understand that you are suggesting we rely on this field's existence to determine if by default the operation should be upsert metadata or delete metadata, right?

@toly11
Copy link
Contributor Author

toly11 commented Dec 1, 2023

@tprouvot
I fixed an issue I found that occurred when setting a number field (or any non string field) to null:
'' is not valid for the type xsd:double

@tprouvot
Copy link
Owner

tprouvot commented Dec 1, 2023

I'm wondering if we need to display API Type to users, maybe we can just leave it as it was and automatically select metadata when the sobject ends with __mdt ? What do you think ?

You're onto something with the idea of auto-selecting Metadata API for __mdt objects behind the scenes, I initially went down that path, as seen in my first commit. However, I realized separating API types makes development and future updates smoother. Here's why:

Action Limitations: With Metadata, we only have upsert and delete options. This differs from the other APIs.

Primary Key Differences: For custom metadata, "DeveloperName" is always the key, unlike "Id" or external IDs in other cases. This simplifies the code for the field mapping logic.

Future Feature Integration: Planning ahead, we might want to show the Metadata option only if users have the required permissions (like 'Customize Application').

Support for additional APIs In the future we might want to add additional APIs, this will be easier with this architecture.

Terminology: Also, we've moved beyond a simple Tooling vs. non-Tooling binary. Now, with Enterprise, Tooling, and Metadata as distinct APIs, it's more accurate and clearer to maintain this division in terms.

This approach might complicate the UI slightly, but it's a trade-off for clearer, more maintainable code and functionality. Your thoughts?

I agree with you, it is cleaner to have a dedicated field for this, I was just worried that some users select this option without knowing what they are doing and create issue after that 😄
OK let's keep it like this you convinced me 👍

@tprouvot
Copy link
Owner

tprouvot commented Dec 1, 2023

Nice updates 👍 I don't know if you've seen my last comment on the Api select input :
"I'm wondering if we need to display API Type to users, maybe we can just leave it as it was and automatically select metadata when the sobject ends with __mdt ?" Since the selection is automatic now, we can keep the previous UI, what do you think ?
Have you tried to update CMDT without MasterLabel ? I have an error when the field is not used
image

Have you tried to update CMDT without MasterLabel ?
I have an error when the field is not used

I double checked, and you're right, it is required in upsert operations also on update. So I understand that you are suggesting we rely on this field's existence to determine if by default the operation should be upsert metadata or delete metadata, right?

No I was mentionning that if this field is not in the used fields, we should disable the button (since we already knwo the operation will fail) just like we already do with DeveloperName field:

image

but with MasterLabel field:
image

image

@toly11
Copy link
Contributor Author

toly11 commented Dec 1, 2023

I was just worried that some users select this option without knowing what they are doing and create issue after that

In any case, I hope I addressed this concern by adding the word "default" to the Enterprise API type
image

@toly11
Copy link
Contributor Author

toly11 commented Dec 1, 2023

No I was mentionning that if this field is not in the used fields, we should disable the button (since we already knwo the operation will fail) just like we already do with DeveloperName field:

I see what you mean. Good point

On the other hand
A. Do we handle other missing required fields like Contact LastName?
B. Where should this message be presented

@tprouvot
Copy link
Owner

tprouvot commented Dec 1, 2023

No I was mentionning that if this field is not in the used fields, we should disable the button (since we already knwo the operation will fail) just like we already do with DeveloperName field:

I see what you mean. Good point

On the other hand A. Do we handle other missing required fields like Contact LastName? B. Where should this message be presented

I don't see your point with the Contact LastName, to me we must disable the action if the minimum required fields are not used (ie Update button is disabled if no 'Id' field)
The error must be the same as those one with MasterLabel field

image image

It is not mandatory since the user will have the info just after hitting the button

@toly11
Copy link
Contributor Author

toly11 commented Dec 2, 2023

I don't see your point with the Contact LastName, to me we must disable the action if the minimum required fields are not used (ie Update button is disabled if no 'Id' field) The error must be the same as those one with MasterLabel field

It is not mandatory since the user will have the info just after hitting the button

My point was that the MasterLabel is more like a required field than a primary key. And currently, Inspector doesn't validate the existence of required fields in insert operations (like the Contact LastName which is required, and the insert button is clickable even if a LastName column wasn't provided).

But on the other hand, the user could technically have an automation that populates the required fields automatically, but custom metadata types do not support automation. So if it's missing it will indeed fail

So, I think it's a good idea. And I will try to implement it.

Edit: I implemented that
image

CHANGES.md Outdated Show resolved Hide resolved
@tprouvot
Copy link
Owner

tprouvot commented Dec 4, 2023

Great ! Thanks for those updates 🙏
Could you resolve conflicts from the releaseCandidate branch, after that I will be able to merge your PR

@toly11
Copy link
Contributor Author

toly11 commented Dec 4, 2023

Great ! Thanks for those updates 🙏 Could you resolve conflicts from the releaseCandidate branch, after that I will be able to merge your PR

I have resolved the conflicts

@tprouvot tprouvot merged commit 4b77ddc into tprouvot:releaseCandidate Dec 4, 2023
@toly11
Copy link
Contributor Author

toly11 commented Dec 4, 2023

Looking forward for everyone to enjoy this new feature! 🤩

@tprouvot, when do you think the next version will come out?

@tprouvot
Copy link
Owner

tprouvot commented Dec 4, 2023

@toly11 Initially I wanted to release it on January but I think that we may have enough new features to release it before.

I created a new extension today on chrome web store to allow users (with extension link) to test the BETA version which correspond to the releaseCandidate branch, I'm thinking of releasing the beta version for a moment (maybe a month) and after this period for all users.

@toly11
Copy link
Contributor Author

toly11 commented Dec 4, 2023

@toly11 Initially I wanted to release it on January but I think that we may have enough new features to release it before.

I created a new extension today on chrome web store to allow users (with extension link) to test the BETA version which correspond to the releaseCandidate branch, I'm thinking of releasing the beta version for a moment (maybe a month) and after this period for all users.

I think it's a great idea!

I suggest listing it privately, and putting the link to it on this repositories' main page (README.md)

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