omise / omise-prestashop

Omise PrestaShop Plugin
https://docs.opn.ooo/prestashop-plugin
MIT License
4 stars 7 forks source link

Develop the setting page by using the template file instead of using the array configuration #10

Closed nimid closed 7 years ago

nimid commented 8 years ago

1. Objective reason

Develop the setting page by using the template file instead of using the array configuration.

At the setting page, it has controls, secret key for test and secret key for live. For one of security best practices which is indicated on the Omise web page, Secure your Account Secret Keys, the type of secret key should be the password not the text.

To implement the type of control to be password and the behavior remain the same as the text, the array configuration is not appropriate. Not only the display style was changed, the core function of PrestaShop is not allowed the value to be displayed on the control that is password type when retrieve the data from the database.

So, to implement the control to be password type, using the template file is more appropriate.

An additional benefit in using the template file is fully customized.

Related ticket: T1062

2. Description of change

The screenshot below shows the setting page that the control of secret key for test and secret key for live are the password type.

screenshot-localhost 2016-09-22 10-54-23

3. Users affected by the change

All

4. Impact of the change

-

5. Priority of change

Normal

6. Alternate solution

-

guzzilar commented 7 years ago

@nimid just only 2 main comments left:

and ok for the rest πŸ‘

guzzilar commented 7 years ago

fyi, there still has some methods that docblock is missing. Here:

thanks! πŸ‘

guzzilar commented 7 years ago

@nimid βœ‹ do you have any rule behind that you named these filename setting.php, checkout_form.php (from #11) instead of Setting.php, CheckoutForm.php?

saw from test classes you used OmiseTest.php, CheckoutFormTest.php.

nimid commented 7 years ago

@guzzilar For a function, Setting.save() https://github.com/omise/omise-prestashop/pull/10/files#diff-726b73960d58daf9fe48ddd631df8a5cR103`, which DocBlock tag you considered it is missing.

nimid commented 7 years ago

@guzzilar

do you have any rule behind that you named these filename

At a table that in the section, File structure for a PrestaShop module. http://doc.prestashop.com/display/PS16/Creating+a+first+module

guzzilar commented 7 years ago

@nimid https://github.com/omise/omise-prestashop/pull/10#issuecomment-272377747

For a function, Setting.save() ... which DocBlock tag you considered it is missing.

thought we can add

/**
 * @return void
 */
guzzilar commented 7 years ago

@nimid https://github.com/omise/omise-prestashop/pull/10#issuecomment-272378189

At a table that in the section, File structure for a PrestaShop module.

checked it, can't find the right answer there πŸ˜“ (saw only they mentioned about only one file, name_of_the_module.php)

Guess you want to make it corresponding with name_of_the_module.php right?

_Btw, just because usually I saw filename that name it as MyClass.php to make it corresponding to its class name class MyClass. But, anyway also saw some PrestaShop's modules named their files in various ways. (some of them are, Do_Something.php, DoSomething.php or do_something.php)_

just wanna understand the idea behind :)

nimid commented 7 years ago

@guzzilar

I agree with @return void but I have researched and found a description.

  1. functions and methods without a return value, the @return tag MAY be omitted here, in which case @return void is implied.
guzzilar commented 7 years ago

https://github.com/omise/omise-prestashop/pull/10#issuecomment-272390907 @nimid I think that will work for __constructor() that we usually skip @return.

Anyway, πŸ‘Œ

guzzilar commented 7 years ago

πŸ‘ πŸ‘ πŸ‘