Manage brands

[adam-code-labx]

As requested #454 Here is the PR to manage brands.

Still to do

• Additional tests
• Product relation table if required?

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
getcandy-2-docs	✅ Ready (Inspect)	Visit Preview	Sep 8, 2022 at 8:34AM (UTC)

[adam-code-labx]

Great work on the filament tables integration @alecritson it is now much easier to create these page resources 😉

[adam-code-labx]

@alecritson Also we're not migrating existing data into the new format, meaning if someone was to upgrade and run the migrations as they are currently, they'd lose all their brands.

Good point 😂 I guess for now we should keep the brand field and create a sync brands command for the user to run after upgrade? Or we could create an action within the brands index to sync brands one time then remove the field?

[adam-code-labx]

Instead of product relation table I have linked the counter for now to filter the products by brand.

[alecritson]

@alecritson Also we're not migrating existing data into the new format, meaning if someone was to upgrade and run the migrations as they are currently, they'd lose all their brands.

Good point 😂 I guess for now we should keep the brand field and create a sync brands command for the user to run after upgrade? Or we could create an action within the brands index to sync brands one time then remove the field?

It's a tricky one, I'm not sure I'm a fan of the approach of having a command since users could forget to run it and then we're doing schema changes outside of migrations. Historically for other projects I've done that kind of thing in the migration itself.

We did introduce a state management routine which listens for when the migrations ended. We could add one for the Illuminate/Database/Events/MigrationStarted event and in the listener, check it's the brands migration running, fetch distinct all the brands and products associated and store them temporarily. Then have the same one run on another event/method and populate the new database...

https://github.com/getcandy/getcandy/blob/develop/packages/core/src/GetCandyServiceProvider.php#L212 https://github.com/getcandy/getcandy/tree/develop/packages/core/database/state

Could look something like:

<?php

namespace GetCandy\Database\State;

use GetCandy\Models\TaxClass;
use Illuminate\Support\Facades\Schema;

class EnsureBrandsAreUpgraded
{
    public function run()
    {
        $brandMapping = get_stored_mapping();
        
        insert_new_brands();
        
        delete_stored_mapping();
    }

    public function migrationStarted()
    {
         $brands = fetch_brands_with_product_ids();
         
         store_brand_mapping();
         
         //
    }

    protected function canRun()
    {
        return true;
    }

    protected function shouldRun()
    {
        return ! Brand::count();
    }
}

Event::listen(
    MigrationStarted::class,
    [EnsureBrandsAreUpgraded::class, 'migrationStarted']
);

Sorry for the crudeness, but what do you think? @glennjacobs any thoughts?

[glennjacobs]

In terms of migrating the data, yes we should stick with our standard approach of state management.

We need to be very careful data cannot be lost. We ideally want to introduce the new fields/tables and populate them with the data before removing the old brand field.

[adam-code-labx]

Ok working on this now.

[adam-code-labx]

@alecritson I have chained this one to existing MigrationsEnded listener with some additional checks inside the new state class. Also before the removing the brand column I added an additional check to see if the legacy count matches up with the brand relation count. So should be good now.

[alecritson]

@adam-code-labx Are you finished up for now? So I can pull this down and give it a test ?

[alecritson]

Also might be worth mentioning @adam-code-labx I've added some table tests, be good to get some in there that match.

[adam-code-labx]

@adam-code-labx Are you finished up for now? So I can pull this down and give it a test ?

Sure I will can add some some tests later tonight.

[alecritson]

@adam-code-labx Pulled down the branch and ran it on the demo store and it seemed to work well 🥳 good work!

My only question, i'm sure @glennjacobs will have an answer too, is should brands be in the side menu or under settings?

[adam-code-labx]

@alecritson Personally I would prefer it grouped on the left especially with the menu grouping which I am using in my hub customisations branch.

Settings I am going to be using the Spatie settings integrated with filament forms package https://github.com/filamentphp/spatie-laravel-settings-plugin/blob/2.x/src/Pages/SettingsPage.php#L33-L37

Dan says we only need those 3 lines as the form package will cover the rest. So expect some more PR's at some point from me for the settings with filament forms.

[alecritson]

Personally I'd hold off doing a PR with filament forms. It would be quite a big undertaking and I don't think we'll be looking to add it any time soon.

[glennjacobs]

Personally I'd hold off doing a PR with filament forms. It would be quite a big undertaking and I don't think we'll be looking to add it any time soon.

I would second that. I think Filament Notifications could be cool though.

[adam-code-labx]

Personally I'd hold off doing a PR with filament forms. It would be quite a big undertaking and I don't think we'll be looking to add it any time soon.

I would second that. I think Filament Notifications could be cool though.

I am talking more for the settings sections. To save time I plan to create artisan commands and dynamically add the new sections to the menu and group them where necessary.

Once done I will perhaps demonstrate the idea to you guys. At this stage I need a way to speed up the creation of the new sections especially the settings. Not really looking to refactor any of your existing forms but might use filament forms for new sections to speed things up.

So anything I end up creating or customising for my clients project. I will submit PR's to suggest to use for the Core or plugin. However my main focus right now is to finish all my clients hub customisations. Filament tables will certainly help with this, so thanks for implementing this one.

My next step is going to be creating the cart section to view carts in the hub and then implement cart rule discounts. Lots todo and only 2 weeks left to finish all the hub additions before jumping on to the front-end integration 😅

[alecritson]

Personally I'd hold off doing a PR with filament forms. It would be quite a big undertaking and I don't think we'll be looking to add it any time soon.

I would second that. I think Filament Notifications could be cool though.

I am talking more for the settings sections. To save time I plan to create artisan commands and dynamically add the new sections to the menu and group them where necessary.

Once done I will perhaps demonstrate the idea to you guys. At this stage I need a way to speed up the creation of the new sections especially the settings. Not really looking to refactor any of your existing forms but might use filament forms for new sections to speed things up.

So anything I end up creating or customising for my clients project. I will submit PR's to suggest to use for the Core or plugin. However my main focus right now is to finish all my clients hub customisations. Filament tables will certainly help with this, so thanks for implementing this one.

My next step is going to be creating the cart section to view carts in the hub and then implement cart rule discounts. Lots todo and only 2 weeks left to finish all the hub additions before jumping on to the front-end integration 😅

I think that's fine if you're extending GetCandy in your own code base. But just a heads up before you do any substantial work on it, if there's a PR with filament forms included it'll likely get rejected. I totally understand that the forms package is great but we do not want to add it to the hub at this point (potentially at all).

Also regarding discounts, there is a PR for it that's been worked on, so instead of doing something from scratch that can be collaborated on if it meets what you need?

[alecritson]

@adam-code-labx Looks like there is a test failing, related to the brands column?

[adam-code-labx]

@adam-code-labx Looks like there is a test failing, related to the brands column?

Just running the tests on my side again and I am now getting this error

Runtime: PHP 8.1.9 Configuration: /Volumes/CODE-LABX/Packages/getcandy/getcandy/phpunit.xml

Error : Class "BladeUI\Icons\BladeIconsServiceProvider" not found

[alecritson]

@adam-code-labx Looks like there is a test failing, related to the brands column?

Just running the tests on my side again and I am now getting this error

Runtime: PHP 8.1.9 Configuration: /Volumes/CODE-LABX/Packages/getcandy/getcandy/phpunit.xml

Error : Class "BladeUI\Icons\BladeIconsServiceProvider" not found

Is that something that Filament tables provides? Have you updated your branch from the latest data tables branch?

[adam-code-labx]

BladeIconsServiceProvider

Solved with vendor/bin/monorepo-builder merge then had to remove composer.lock then run composer i Also fixed the product test and added a test for the brands table.

[glennjacobs]

@adam-code-labx You may have seen, but unfortunately we are not going to be using Filament Tables package. It was impossible to get the performance from it we needed, amongst other issues.

We are working on a solution for this, however, as we do need a good datatable. We will update you when we have more to say.

[adam-code-labx]

@glennjacobs ok disappointing I have never really come across performance issues directly related to Filament Tables.

Anyway this PR does not have to depend on filament. Do you want me to update to use the current GC table format?

[glennjacobs]

@glennjacobs ok disappointing I have never really come across performance issues directly related to Filament Tables.

Anyway this PR does not have to depend on filament. Do you want me to update to use the current GC table format?

It was disappointing. We thought about addressing it with some PRs, but it would have taken the project in a potentially different direction for Filament, so I don't think it would have been welcomed. Also, as you know, we're keen to really minimise the use of third-party packages, so we'll only bring one in when it's spot-on.

In terms of this PR, I would probably look to update it when our new datatables PR is made. As I don't believe we'll be making a release before datatables and search is ready.

[wychoong]

when our new datatables PR is made. As I don't believe we'll be making a release before datatables and search is ready.

can the Brand go in first as datatables and search seems huge to be blocker for other features to be merged in. or at least datatables/search can be broken down into smaller pieces and release into develop, since GC is not in a stable state yet it doesn't need to wait for everything to be ready at once

[adam-code-labx]

when our new datatables PR is made. As I don't believe we'll be making a release before datatables and search is ready.

can the Brand go in first as datatables and search seems huge to be blocker for other features to be merged in. or at least datatables/search can be broken down into smaller pieces and release into develop, since GC is not in a stable state yet it doesn't need to wait for everything to be ready at once

@glennjacobs I agree with @wychoong The idea for the develop branch I thought was for all the developers to help test before any releases. Can you please change the base branch back to develop and I will rebase and continue without the data tables. Thanks

[alecritson]

Hey @adam-code-labx I've changed the base for you, I agree it would be good to get this merged in to develop asap.

[adam-code-labx]

@wychoong @alecritson Should be good to go now.

Had to recreate as creating patch from various commits and trying to drop the datatables failed 😭 The tests are all passing but I didn't have time to do a migrate fresh or test the EnsureBrandsAreUpgraded.

Copied everything from previous commits file history so shouldn't have any problems just needs testing from your side. Let me know if there are any issues and I will sort out any fixes required night.

[alecritson]

Hey @adam-code-labx

I've pulled this down this morning and had a little look, just a couple of questions/points:

1. When clicking "cancel" on the create modal, the brand name doesn't reset itself.
2. If I delete a brand which has products, an exception is thrown

How did you imagine this being handled? What if, in the instance of products existing, there is a message stating this needs to be sorted before it can be deleted?

[adam-code-labx]

Hey @adam-code-labx

I've pulled this down this morning and had a little look, just a couple of questions/points:

1. When clicking "cancel" on the create modal, the brand name doesn't reset itself.
2. If I delete a brand which has products, an exception is thrown

How did you imagine this being handled? What if, in the instance of products existing, there is a message stating this needs to be sorted before it can be deleted?

@alecritson Sorry only just seen this one. Will get that sorted tonight. I would say we should allow delete with a warning to say the brand will be detached from all products.