gnikyt / laravel-shopify

A full-featured Laravel package for aiding in Shopify App development
MIT License
1.24k stars 374 forks source link

Allow for custom "shops" table for automatic migrations #1192

Closed nahid closed 2 years ago

nahid commented 2 years ago

laravel-shopify allowing custom auth guard in their latest version. But it not support to allowing migration with custom table for shops in automatic migration.

This PR is allowing developer's to set their own shops table in config to execute migration with given name.

nahid commented 2 years ago

@osiset, I have already rewritten the code based on your change request. Please review the PR again. Thank you

gnikyt commented 2 years ago

@nahid I am fine at a quick glance except for the fact that the table function is doing more than one thing.

It is named getShopsTable but also the the ability to return a string for a column. I'd split it into two functions... getShopsTable, and shopColumnFor or something?

or... return an array or object for getShopsTable.

Something like:

// Method A
public static function getShopsTable(): array
{
        $shopTable = Util::getShopifyConfig('table_names.shops') ?? 'users';
        $foreignId = Str::singular($shopTable) . '_id';
        return ['table' => $shopTable, 'foreign' => $foreignId];
}

// or .... Method B
public static function getShopsTable(): array
{
        $shopTable = Util::getShopifyConfig('table_names.shops') ?? 'users';
        $foreignId = Str::singular($shopTable) . '_id';
        return [$shopTable, $foreignId];
}

Usage could be something like:

$table = Util::getShopsTable()["table"];

// or ... Method B
list($table) = Util::getShopsTable();
// or ... Method B
$table = Util::getShopsTable()[0];

Or, if that feels messy, then split into two functions.

nahid commented 2 years ago

I think different method for each functionality will added more readability and developer experience as well. Here is the example:

Util::getShopsTabel(): string

Util::getShopsTableForeignKey(): string
gnikyt commented 2 years ago

@nahid I agree. Go ahead with it :)

nahid commented 2 years ago

@osiset, I have pushed the following changes, Please review again

gnikyt commented 2 years ago

Solid. I'll approve letting Actions run and we'll see what happens. Probably will need test added to Until tests to ensure coverage is there.

nahid commented 2 years ago

Solid. I'll approve letting Actions run and we'll see what happens. Probably will need test added to Until tests to ensure coverage is there.

Maybe tests/Storage/Queries/ChargeTest.php file needs to be updated to code coverage. Please let me know if there is anything I can do. Thank you

gnikyt commented 2 years ago

@nahid Tests pass, coverage didnt change as well, so thats OK. Linting failed though, please see the Action for details.

nahid commented 2 years ago

@nahid Tests pass, coverage didnt change as well, so thats OK. Linting failed though, please see the Action for details.

@osiset, I just pushed another commit with fixes, please check

gnikyt commented 2 years ago

@nahid Still is failing the lint.

Locally, you can vendor/bin/php-cs-fixer fix --diff --dry-run to see what needs to be changed, and vendor/bin/php-cs-fixer fix if you're OK with automatic changes.

nahid commented 2 years ago

@osiset please check the latest commits

gnikyt commented 2 years ago

Tests now pass, linting is good. @nahid

@Kyon147 I'm good with this one too if you are, seems like a feature thats been asked about before a few times.

Kyon147 commented 2 years ago

Tests now pass, linting is good. @nahid

@Kyon147 I'm good with this one too if you are, seems like a feature thats been asked about before a few times.

Yeah looks good to me too

gnikyt commented 2 years ago

@nahid Please review once more, and if you're good we'll merge this in.

nahid commented 2 years ago

@nahid Please review once more, and if you're good we'll merge this in.

@osiset sure