search

omise / omise-prestashop

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

Citi #80

Closed jonrandy closed 4 years ago

jonrandy commented 4 years ago

1. Objective

Adds Citipoints payment to Prestashop

2. Description of change

Added the few files necessary to add the new payment method (proves the refactoring was worthwhile! 😄 ). Updated OmisePHP and updated API version (was necessary to allow Citi to work) .

3. Quality assurance

Checked all admin and payment function relating to Citi worked ok. Also made sure all previous payment methods still working (due to the OmisePHP and API version changes)

4. Impact of the change

Payment with Citi points now available in Prestashop!

5. Priority of change

Normal

jonrandy commented 4 years ago

@jonrandy could you add screenshots of this change as well? as it provides new UIs to the plugin : )

Not worth it - just adds an on/off switch identical to that from other payment methods. UI changes are self explanatory

guzzilar commented 4 years ago

Not worth it - just adds an on/off switch identical to that from other payment methods. UI changes are self explanatory

Worth it for what?. The changes have modified the admin page and introduce a new payment at the check out page. This is just a normal code review process of providing details of [2. description of change]

jonrandy commented 4 years ago

I think images should only ever be necessary if changes are hard to spot or not obvious. If the PR states it’s adding a payment method and says it is changing/adding an admin template it should be obvious where the changes will appear - at the checkout and on the admin page. As for showing what they look like at the front end - this is dependent upon the theme being used on the store - which could be different between sites - so the changes in the files give a better indication of the change - i.e. the HTML will show any new fields etc.

guzzilar commented 4 years ago

I think images should only ever be necessary if changes are hard to spot or not obvious.

I think it is helping on a better & clear communication on what changes because reviewers or anyone that read this pull request may have a different level of understanding on the PrestaShop UI and what has been added.

But if you think it is not necessary then ok.