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

[ON HOLD][Shopify] Shopify allow use of customer creation and update #24967

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BernhardKloibmueller
Copy link
Contributor

We would like to make a custom mapping with different fields to identify existing customers and decide based on them to create a new customer from a shopify customer.

Since the enum "Shpfy Customer Mapping" is extensible we thought this should be possible,
Unfortunately the codeunits for creating and updating customers are only accessible internally and there is no other interface to change a customer based on shopify data.

We would like to avoid writing our own Create Customer and Update Customer codeunits just because they are only internal allowed. From our point of view there is no particular reason to not allow these codeunits to be used by an extension.

If we understood the usage of a codeunit wrong I would accept any clarification.
Thank you in advance.

@BernhardKloibmueller
Copy link
Contributor Author

@microsoft-github-policy-service agree company="NAVAX Consulting GmbH"

begin
ShopifyCustomerResult := ShopifyCustomer;
end;

internal procedure SetAllowCreate(Value: Boolean)
procedure SetAllowCreate(Value: Boolean)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should have a good documentation for all procedures that aren't marked as internal.

@pri-kise
Copy link
Contributor

pri-kise commented Oct 9, 2023

@BernhardKloibmueller could you maybe add a small example (code draft) for the extensibility?

I try to understand how we can improve the following documentation:
https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-extending-shopify

with this new extensiblity.

@pri-kise
Copy link
Contributor

pri-kise commented Oct 9, 2023

Additionally I'm not sure if you should simple remove the Internal property.

Maybe we should make use of the facade pattern and add new codeunits for this reason.

@JesperSchulz and @AndreiPanko for help on this.

@AndreiPanko
Copy link
Contributor

This is a difficult one.
I agree that it is unfortunate that while "Shpfy Customer Mapping" is extensible, this is not used in Create/Map loggic. So new options would not make any different.

Would be great to see PR that replaces the current implementation with extensible one. That's of course more work.

Usually functions of extension are not available from outside and I would prefer to keep it this way to reduce surface. However I don't see why not to expose some of the methods via facade pattern we have (see modules in system app).
For example I'm reluctant to open SetShop method as it is not really needed as shopify customer record now have reference to shop.

@AndreiPanko AndreiPanko added the follow up Follow up is needed label Oct 10, 2023
@BernhardKloibmueller
Copy link
Contributor Author

@AndreiPanko @pri-kise
Thank you for the responses.

Then I will not make any more updates until you decide how to proceed with the topic.
When you confirm it, then I will do the changes in this PR

What would we like to achieve in detail:
We want not to reuse customers existing in business central for shopify mapping. we only want to create new customers in business central. This is because our customer has already a custom webshop with many different benefit systems which should not interfere. This behavior we now could achieve with one of the existing events which is used in the E-Mail mapping option.

But we needed to refuse a feature because we could not implement it yet. This was Mapping based on if the customer was created by shopify itself. This we wanted to make possible with the interface.

@pri-kise :
Regarding the documentation:
At them moment this is missing the customer events completely. Maybe you can add an example to fill additional customer data.

for example something like this:

    [EventSubscriber(ObjectType::Codeunit, Codeunit::"Shpfy Customer Events", OnAfterCreateCustomer, '', false, false)]
    local procedure OnAfterCreateCustomer(Shop: Record "Shpfy Shop"; var ShopifyCustomerAddress: Record "Shpfy Customer Address"; var Customer: Record Customer);
    begin
        Customer."House Number" := GetHouseNumberFormAddress(ShopifyCustomerAddress."Address 1");
        Customer."Street" := GetStreetNameFormAddress(ShopifyCustomerAddress."Address 1");
        Customer.CalculateAddressFromStreetAndHouseNumber();
        Customer.Modify(false);

        ShopifyCustomerAddress."Address 1" := Customer.Address;
        ShopifyCustomerAddress.Modify(false);
    end;

Is this helpful or do you need more?
Thank you all in advance.

@JesperSchulz JesperSchulz changed the title Shopify allow use of customer creation and update [Shopify] Shopify allow use of customer creation and update Mar 20, 2024
Copy link
Contributor

Could not find linked issues in the pull request description. Please make sure the pull request description contains a line that contains 'Fixes #' followed by the issue number being fixed. Use that pattern for every issue you want to link.

@JesperSchulz JesperSchulz added the Integration GitHub request for Integration area label Apr 22, 2024
@JesperSchulz JesperSchulz changed the title [Shopify] Shopify allow use of customer creation and update [ON HOLD][Shopify] Shopify allow use of customer creation and update Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
follow up Follow up is needed Integration GitHub request for Integration area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants