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

#Show Holder Name Field Fix - When Config is set to NO/YES in Magento admin #1373

Merged
merged 4 commits into from Feb 22, 2022

Conversation

dejankar
Copy link
Contributor

@dejankar dejankar commented Feb 18, 2022

Description

  • Javascript expects bool to be returned on the frontend and to avoid type conversion we can get bool type directly from config.

  • getConfigData() function variable flag is false by default, so IsSetFlag function is ignored in this case.

  • Since has_holder_name & has_holder_name required are yes/no magento fields return, $this->scopeConfig->isSetFlag()will return true/false by default if flag is set to true under getHasHolderName() & getHasHolderNameRequired().

Tested scenarios

  • Tested both scenarios when the Show holder name field for card payment methods is set to NO and YES.
  • The frontend form is rendered properly without the need for type conversion in the javascript.

Fixed issue:
#1336
#1345

- Javascript expects bool to be returned on frontend and to avoid type conversion we can get bool type directly from config.

- getConfigData flag is false by default.

- Since has_holder_name & has_holder_name required are yes/no magento fields return $this->scopeConfig->isSetFlag will return true/false by default if flag is set to true under getHasHolderName() & getHasHolderNameRequired().
@dejankar dejankar changed the title #Show Holder Name Field Fix - When Config is set to NO in Magento admin #Show Holder Name Field Fix - When Config is set to NO/YES in Magento admin Feb 18, 2022
@Morerice
Copy link
Contributor

Hi @dejankar,

Thanks for opening this PR. As you rightly pointed out this is a cleaner solution to my older PR. Hence, could you please also revert the changes of that PR and include it in this PR? Since those changes will be redundant once we merge your PR.

Also could you please remove the binary file which I think was accidentally added?

Best regards,
Jean
Adyen

… admin

Description

Javascript expects bool to be returned on the frontend and to avoid type conversion we can get bool type directly from config.

getConfigData() function variable flag is false by default, so IsSetFlag function is ignored in this case.

Since has_holder_name & has_holder_name required are yes/no magento fields return, $this->scopeConfig->isSetFlag()will return true/false by default if flag is set to true under getHasHolderName() & getHasHolderNameRequired().

Tested scenarios

Tested both scenarios when the Show holder name field for card payment methods is set to NO and YES.
The frontend form is rendered properly without the need for type conversion in the javascript.
Fixed issue:
Adyen#1336
Adyen#1345
@dejankar
Copy link
Contributor Author

Hi @dejankar,

Thanks for opening this PR. As you rightly pointed out this is a cleaner solution to my older PR. Hence, could you please also revert the changes of that PR and include it in this PR? Since those changes will be redundant once we merge your PR.

Also could you please remove the binary file which I think was accidentally added?

Best regards, Jean Adyen

Hi Jean,

I hope this is what you mean by reverting changes from your commit and adding it to my commit?

In case I'm wrong please specify the exact commit numbers that need to be changed or reverted.

Best Regards,
Dejan

@Morerice
Copy link
Contributor

Hi @dejankar,

Yes, that was exactly what I meant. Thanks a lot for your quick response!

Best regards,
Jean
Adyen

@sonarcloud
Copy link

sonarcloud bot commented Feb 22, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Morerice Morerice merged commit a684d97 into Adyen:develop Feb 22, 2022
@Morerice Morerice mentioned this pull request Apr 4, 2022
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.

None yet

3 participants