TIPOFF / locations

Laravel Package for locations in markets
MIT License
0 stars 1 forks source link

Restructure Markets & Locations #31

Closed drewroberts closed 3 years ago

drewroberts commented 3 years ago

The Locations model has too much application-specific data and should be spread out to the related packages that need things from this model. @pdbreen explained it well in his proposal:

Proposal

Location is a meaningful concept to many other models, even when there is only a single location. An escape room company using these packages may not have multiple locations, but they still have a single location. An Ecommerce company may not hold inventory in multiple locations, but they still have inventory in a location.

As a result, direct dependencies on Location should be allowed. But, with changes.

Changes

The Location model as it exists currently is too smart - it contains too much knowledge about things that may not be part of the system. For example, it contains knowledge of taxes and payment (stripe settings).

To fix this, the Location model itself should not be the access point for this package-specific information. Instead, the relations should be inverted.

For example, instead of storing payment information within the location model, the Payment package should have a required dependency on Location. With this dependency, the Payment package should instead define its own tables to hold location-specific information.

Schema::create('payment_location_settings', function (Blueprint $table) {
    $table->id();
    $table->foreignIdFor(Location::class)->unique();    // NOTE - unique() added if there should be exactly one record per location
    // .. payment settings that are location specific
    // .. Creator & Updater
    $table->timestamps();
});

Benefits

Allowing a dumbed down location to exist allows other packages that have location related logic the ability to implement this logic consistently for both single location deployments and multiple location deployments. It also provides a path in which a single location deployment could later scale up to a multi-location deployment.

Impact

Inversions of a true Location hasOne relation adds some minor complexity. Responsibility for ensuring that there is exactly one per location shifts to the package. Including a unique key on the location will allow the DB to flag errors, but this safety net doesn't eliminate the responsibility for the package to provide an appropriate API to avoid duplication.

Flipping the relationships will likely have an impact on navigation within Nova. For example, instead of navigating to Location to establish product tax detail, you would instead navigate to taxes and set a location on a tax detail record. There should be ways around this to add optional fields in Nova on the Locations resource.

pdbreen commented 3 years ago

Cleanup should include removal of all use of booking_tax_id, product_tax_id, booking_fee_id and booking_fee_id in the Location model, nova resource and factories. There should be no remaining references to app('fee') or app('tax') anywhere in the code - migrations, tests, factories, models, nova resources, etc, etc