Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BusinessCategory #23 #64

Merged
merged 26 commits into from
Mar 23, 2021
Merged

Conversation

djamesfar
Copy link
Contributor

create BusinessCategory model, categories in migration; business_category_place join table migration; add relations to Place and BusinessCategory

@drewroberts drewroberts marked this pull request as draft March 17, 2021 15:16
@drewroberts drewroberts self-requested a review March 17, 2021 15:16
@@ -12,7 +12,7 @@ public function up()
{
Schema::create('business_category_place', function (Blueprint $table) {
$table->id();
$table->foreignIdFor(app('business_category'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add this class to the tipoff/support config. Can you do a PR for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

Comment on lines 13 to 19
Schema::create('business_category_place', function (Blueprint $table) {
$table->id();
$table->foreignIdFor(\Tipoff\Seo\Models\BusinessCategory::class);
$table->foreignIdFor(app('place'));
$table->timestamps();

$table->unique(['business_category','place']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment here:

There should be a polymorphic relationship between business_categories and place_details and gmb_details

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this business_category_place table should not exist. A new table for the polymorphic relationships should be created. I'm not sure what to call it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the -able? detailable or categorizable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, let's use categorizable. I don't know what the name of that table should be. Maybe businesscategory_records like we did in the tipoff/statuses pacakge

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use the business_categories table and make it a morph for anything that needs a business category? Would just have name, categorizable_id, categorizable_type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I want that to be unique. I want to know what Business Categories are morphed to each. Not have 500 business categories for the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have a location with a morphed business category, that will be unique to that model: categorizable_id=<location_id> categorizable_type='/tipoff/Locations/Models/Location', name=<gmb_category>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. I don't want it to be unique to the model. I want business_cateogries to be unique names and another model that will be unique to the model.

@drewroberts
Copy link
Member

Copy link
Member

@drewroberts drewroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 👾

@drewroberts drewroberts marked this pull request as ready for review March 23, 2021 00:36
@drewroberts drewroberts merged commit dd7d096 into main Mar 23, 2021
@drewroberts drewroberts deleted the omnia/features/23-business-categories branch March 23, 2021 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants