lat9 / upsoauth

A Zen Cart UPS shipping module that makes use of UPS' RESTful/Oauth API
GNU General Public License v2.0
3 stars 3 forks source link

not really doing what i want... #9

Closed proseLA closed 1 year ago

proseLA commented 1 year ago

cindy, while i appreciate the work that went into this class UpsOAuthApi, i am not really sure it is doing what i want.

for example, looking at this bit of code, how could i have a 2nd UPS account, and override the handling fee?

https://github.com/lat9/upsoauth/blob/f828fee02d789fa56c21c200a94dfc2fd72da1f5/includes/modules/shipping/upsoauth/UpsOAuthApi.php#L403-L409

it will always be using the constants defined for the original UPS account as opposed to being able to override it similar to the way i did with the upsxml implementation.

perhaps you could explain how i could have 2 ups accounts with different handling fees, and only show 1 to the customer based on the weight of the package?

it just seems that what is going on here is unnecessarily complex when all i really wanted was a notifier in the quote method to enable or disable quoting as i indicated here:

https://github.com/lat9/upsxml/issues/36

using all of the upsoauth constants in this class UpsOAuthApi means i need to override that class, and rebuild the whole thing.

i guess i am not seeing it. perhaps you can help me see the light on this one.

lat9 commented 1 year ago

What's missing is a method via which the handling fee and all other configuration settings can be overridden. Then you'd be able to specify an overridden upsoauth api-class that only provides the handling-fee value override method.

proseLA commented 1 year ago

What's missing is a method via which the handling fee and all other configuration settings can be overridden. Then you'd be able to specify an overridden upsoauth api-class that only provides the handling-fee value override method.

again, i'm not sure what the point of this new class is. it would be one thing if it was a class that could be re-used by multiple ups shipping classes; but it can not. each new shipping class requires it's own extension of that api class to override whatever configuration settings need to be changed. what's the point? if one wanted to pursue having a truly useful api class, it would not have hard coded constants from a specific shipping class.

in the previous xml implementation, all i/we needed was that notifier. and the new class, for the most part, only required the construct method:

class upsxml2 extends upsxml
    {
        function __construct()
        {
            parent::__construct();
            $this->code = 'upsxml2';
            if (IS_ADMIN_FLAG === true) {
                $this->title = 'UPS 2nd Account v' . $this->moduleVersion;
            }
            if (!defined('MODULE_SHIPPING_UPSXML2_SHIPPER_NUMBER')) {
                $this->install();
            }
            if (!defined('MODULE_SHIPPING_UPSXML2_RATES_HANDLING')) {
                $this->install();
            }
            if (!defined('MODULE_SHIPPING_UPSXML2_RATES_SORT_ORDER')) {
                $this->install();
            }
            $this->upsShipperNumber = MODULE_SHIPPING_UPSXML2_SHIPPER_NUMBER ?? '';
            $this->handling_fee = MODULE_SHIPPING_UPSXML2_RATES_HANDLING;
            $this->sort_order = (int) MODULE_SHIPPING_UPSXML2_RATES_SORT_ORDER;
        }

(i did not include the install method and keys method here for brevity.)

having an api class that makes use of the constants associated with 1 specific shipping method really hamstrings that api class. passing the shipping class information to the api where the constants have already been embedded into the shipping class makes so much more sense to me.

lat9 commented 1 year ago

Right, I'm in the process of providing various protected 'getSomething()' methods that (for the base class) return the upsoauth constants, but can be overridden by an API class' extension.