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.

Sign up for GitHub

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

Jump to bottom

Check if WC_Facebookcommerce_Info_Banner is used, and possibly delete if we really don't need it. #1981

Open
budzanowski opened this issue May 27, 2021 · 1 comment · May be fixed by #2435
Open

Check if WC_Facebookcommerce_Info_Banner is used, and possibly delete if we really don't need it. #1981

budzanowski opened this issue May 27, 2021 · 1 comment · May be fixed by #2435
Assignees
@message-dimke @budzanowski
Labels
good first issue needs investigation type: enhancement The issue is a request for an enhancement.

Comments

@budzanowski
Copy link
Collaborator

I suspect that the class WC_Facebookcommerce_Info_Banner is not used at all.
I suspect this because the logic that triggers the use of the banner class relies on
get_external_merchant_settings_id function that gets the OPTION_EXTERNAL_MERCHANT_SETTINGS_ID option.
From what I could find this option is never set, except for some old plugin version update code.

We need to verify if this is really not used. If it is not we should check if we maybe should use it.
I don't really know what an External Merchant Settings ID is.
If we don't need it we should remove all of the class code and the code and the code that refers to OPTION_EXTERNAL_MERCHANT_SETTINGS_ID in any way.
@budzanowski budzanowski added good first issue needs investigation type: enhancement The issue is a request for an enhancement. type: question The issue is a question about how the product works. labels May 27, 2021
@budzanowski budzanowski mentioned this issue May 27, 2021
Merged
@haszari
Copy link
Member

haszari commented May 27, 2021

We need to verify if this is really not used. If it is not we should check if we maybe should use it.

Sounds like a low priority, but if we want more data to confirm this is unused, we might be able to add tracking for the relevant settings. Probably overkill to add tracking, but thought I'd mention it.
This was referenced Oct 19, 2022
Merged
Closed
@message-dimke message-dimke linked a pull request Jan 4, 2023 that will close this issue
Draft
@layoutd layoutd removed the type: question The issue is a question about how the product works. label Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue needs investigation type: enhancement The issue is a request for an enhancement.
4 participants
@layoutd @haszari @message-dimke @budzanowski