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

Column select #776

Merged
merged 25 commits into from
Jun 8, 2022
Merged

Column select #776

merged 25 commits into from
Jun 8, 2022

Conversation

inmanturbo
Copy link
Contributor

This feature uniquely identifies each table for column selections stored in the session. It also allows for columns selections to be put into the query string, so that selections can be bookmarked.

The tldr; is that we now have a fingerprinting method that uses static::class to generate a key, or slug for each component, to be used as an alias for $tableName in the session and query string arrays, which is always the same for the same component, and always different for another.

    /**
     * returns a unique id for the table, used as an alias to identify one table from another session and query string to prevent conflicts
     */
    public function dataTableFingerprint(): string
    {
        $className = str_split(static::class);
        $crc32 = sprintf('%u', crc32(serialize($className)));

        return base_convert($crc32, 10, 36);
    }
    /**
     * Set the custom query string array for this specific table
     *
     * @return array|\null[][]
     */
    public function queryString(): array
    {
        if ($this->queryStringIsEnabled()) {
            return [
                $this->getTableName() => ['except' => null, 'as' => $this->dataTableFingerprint()],
                'userSelectedColumns' => ['except' => null, 'as' => $this->dataTableFingerprint() . '-c']
            ];
        }

        return [];
    }

This produces urls that look something like this:
http://table-demo.test/?11mbmzx[filters][year]=2022&11mbmzx-c[0]=year

Where 11mbmzx is the fingerprint, or unique alias for the component.

The dynamic link above will apply a filter with the key of year and value 2022 and select to only have the year column displayed.

checking only the year column in the columns▼ dropdown on a table fingerprinted 11mbmzx will put 11mbmzx-c[0]=year into the query string. it will also put the following array into the session:

11mbmzx-columnSelectEnabled => array:1 [▼
0 => year
]

Query strings will be evaluated first to determined which columns should be displayed, after that the session, followed by the default.

closes #760

@inmanturbo inmanturbo requested a review from rappasoft May 14, 2022 07:59
@inmanturbo
Copy link
Contributor Author

inmanturbo commented May 14, 2022

These changes are backwards compatible, however we should consider the following:

  • It may be worth considering to replace all the calls to getTableName() (for wire:ids) in the views with the new dataTableFingerprint()
  • Users with more than one instance of the same component in the same app/site could still run into issues with the columns in the session, even on different pages, unless they implement their own dataTableFingerprint() method that maybe also appends the last segment of the route or URL or something like that.
  • some users could have stale links or sessions following the update (but that should clear up after the first visit).

@inmanturbo inmanturbo mentioned this pull request May 14, 2022
@rappasoft
Copy link
Owner

I haven't tested yet but it looks good. Just curious, why add to query string instead of boot:

  1. Still alias getTableName in queryString but instead of userSelectedColumns change boot to:
$this->{$this->tableName}` = [
    'sorts' => $this->{$this->tableName}['sorts'] ?? [],
    'filters' => $this->{$this->tableName}['filters'] ?? [],
    'columns' => $this->{$this->tableName}['userSelectedColumns'] ?? [],
];

Or something like that, unless I'm missing something, but it's cleaner to me that its under the same table key in the URL and not its own global thing?

@inmanturbo
Copy link
Contributor Author

inmanturbo commented May 14, 2022

I think it could be done that way yes. But with the fingerprint (and alias*) now the $this->{$this->tableName} syntax is really no longer required for multiple tables to work on the same page. So the user doesn't have to manually assign properties anymore. It could possibly be removed everywhere to simplify the code in the next major version and existing calls were only kept here as a sort of extra careful compatibility practice. I'm a little hesitant to add more calls to $this->{$this->tableName}. However it should work.

We also may have to check everywhere to make sure the array key $this->{$this->tableName}['userSelectedColumns']
is set to avoid errors. So for example I think the following?

        // Set to either the default set or what is stored in the session
        $this->selectedColumns = count($this->userSelectedColumns) > 0 ?
            $this->userSelectedColumns :
            session()->get($this->getColumnSelectSessionKey(), $columns);

Becomes?

        // Set to either the default set or what is stored in the session
        $this->selectedColumns = (isset($this->{$this->tableName}['userSelectedColumns']) && count($this->{$this->tableName}['userSelectedColumns']) > 0) ?
            $this->{$this->tableName}['userSelectedColumns'] :
            session()->get($this->getColumnSelectSessionKey(), $columns);

*edit

@inmanturbo
Copy link
Contributor Author

Of course I could be wrong here. I'll have to run some tests using the boot() method outlined above.

@inmanturbo
Copy link
Contributor Author

inmanturbo commented May 14, 2022

Ah, I see. Works like this:

        // Set to either the default set or what is stored in the session
        $this->selectedColumns = count($this->{$this->tableName}['columns']) > 0 ?
            $this->{$this->tableName}['columns'] :
            session()->get($this->getColumnSelectSessionKey(), $columns);

I'm going to push a change following your suggestion. It is a little better, I think.

Edit: We can always simplify $this->{$this->tableName} syntax later if it comes to that.

@inmanturbo inmanturbo marked this pull request as draft May 14, 2022 17:54
@inmanturbo inmanturbo marked this pull request as ready for review May 14, 2022 18:25
@rappasoft
Copy link
Owner

I mean i'm all for removing that syntax if it's backward compatible.

@inmanturbo
Copy link
Contributor Author

@rappasoft

I haven't tested yet but it looks good. Just curious, why add to query string instead of boot:

  1. Still alias getTableName in queryString but instead of userSelectedColumns change boot to:
$this->{$this->tableName}` = [
    'sorts' => $this->{$this->tableName}['sorts'] ?? [],
    'filters' => $this->{$this->tableName}['filters'] ?? [],
    'columns' => $this->{$this->tableName}['userSelectedColumns'] ?? [],
];

Or something like that, unless I'm missing something, but it's cleaner to me that its under the same table key in the URL and not its own global thing?

@rappasoft done and tested. (phpunit as well as user story with two tables on same page). Please have a look when you get a chance.

@rappasoft
Copy link
Owner

Don't have too much time to test but I'm getting some funky behavior when I deselect a column:

Cannot assign bool to property Rappasoft\LaravelLivewireTables\DataTableComponent::$selectedColumns of type array

@inmanturbo
Copy link
Contributor Author

Ok thanks. I'll have a look when I get a chance.

@inmanturbo inmanturbo marked this pull request as draft May 14, 2022 18:43
@inmanturbo
Copy link
Contributor Author

Ok so looks like I can replicate the error using the demo. Looks like there is indeed some strange behavior, specifically when the columns aren't actually database columns. The check boxes don't have a value, and the value put into the session is null. I'm using $column->getField() in the checkbox. I supppose getField() must return null if there isn't a database field.

@inmanturbo
Copy link
Contributor Author

Don't have too much time to test but I'm getting some funky behavior when I deselect a column:

Cannot assign bool to property Rappasoft\LaravelLivewireTables\DataTableComponent::$selectedColumns of type array

This was due to the use of getField() being used to identify selected columns in the checkboxes. This has been fixed by adding a getSlug() helper in trait ColumnHelpers , which makes a slug from the title, and using that. This results in the query string using slugged Titles, which is probably better anyway than showing the actual database column names on the frontend.

@inmanturbo inmanturbo marked this pull request as ready for review May 14, 2022 20:06
@inmanturbo
Copy link
Contributor Author

Here's how it must have worked before:

➜  laravel-livewire-tables-demo git:(master) ✗ php artisan tinker                   
Psy Shell v0.11.4 (PHP 8.1.5 — cli) by Justin Hileman
>>> md5(null)
PHP Deprecated:  md5(): Passing null to parameter #1 ($string) of type string is deprecated in laravel-livewire-tables-demoeval()'d code on line 1
=> "d41d8cd98f00b204e9800998ecf8427e"
>>> 

🤔

@inmanturbo inmanturbo marked this pull request as ready for review May 16, 2022 17:51
@inmanturbo
Copy link
Contributor Author

@rappasoft due to the extensibility of working with the configure() method, this actually took less time than I expected! Please look it over when you can.

@inmanturbo
Copy link
Contributor Author

inmanturbo commented May 16, 2022

@Ceepster14 I think we can consider this branch pretty stable at this point. Is there any chance you could test this in your project and confirm that it close #760 before we merge it?

composer require rappasoft/laravel-livewire-tables:column-select-dev

@Ceepster14
Copy link

Hi guys,

Great work! I've given this fix a bit of a thrashing and it all seems to hold up. No more missing columns. Thanks for all your efforts!

Chris

@inmanturbo inmanturbo marked this pull request as draft May 17, 2022 13:47
@inmanturbo
Copy link
Contributor Author

inmanturbo commented May 17, 2022

unfortunately one part of this feature still isn't working properly:

public function configure(): void
{
    $this->setQueryStringAlias('users-table2'); // defaults to $tableName
}

This indeed sets an alias, however on page refresh or a fresh visit it clears the query string. I thought that maybe it was because I hadn't added an actual public property to the DataTableComponent for $queryStringAlias. But I've tried adding it and it doesn't fix the problem.

Here is how the alias is set during configuration:

    public function setQueryStringAlias(string $queryStringAlias): self
    {
        $this->queryStringAlias = $queryStringAlias;

        return $this;
    }

And here is how it is used in the query string:

    public function queryString(): array
    {
        if ($this->queryStringIsEnabled()) {
            return [
                $this->getTableName() => ['except' => null, 'as' => $this->getQueryStringAlias()],
            ];
        }

        return [];
    }

And here is the getQueryStringAlias method

    public function getQueryStringAlias(): string
    {
        return $this->queryStringAlias ?? $this->getTableName();
    }

So far I've tried adding public $queryStringAlias to the component:

    public array $table = [];
    public $theme = null;
+   public $queryStringAlias = null;

But this doesn't seem work. Any ideas or help would be appreciated. Marking this as a draft for now. WIP

@inmanturbo
Copy link
Contributor Author

Hmm ... Looks like it's back to the same problem as here livewire/livewire#4300.

hard coding the alias works fine:

public $queryStringAlias = 'alias';

but trying to set it at runtime doesn't seem to work.

@inmanturbo
Copy link
Contributor Author

This seems to have done the trick. Any reason we can't do this?

    /**
     * Runs on every request, immediately after the component is instantiated, but before any other lifecycle methods are called
     */
    public function boot(): void
    {
        $this->{$this->tableName} = [
            'sorts' => $this->{$this->tableName}['sorts'] ?? [],
            'filters' => $this->{$this->tableName}['filters'] ?? [],
            'columns' => $this->{$this->tableName}['columns'] ?? [],
        ];
        
        // Set the filter defaults based on the filter type
        $this->setFilterDefaults();
+      $this->configure();
    }

    /**
     * Runs on every request, after the component is mounted or hydrated, but before any update methods are called
     */
    public function booted(): void
    {
-      $this->configure();
        $this->setTheme();
        $this->setBuilder($this->builder());
        $this->setColumns();

        // Make sure a primary key is set
        if (! $this->hasPrimaryKey()) {
            throw new DataTableConfigurationException('You must set a primary key using setPrimaryKey in the configure method.');
        }
    }

@inmanturbo inmanturbo marked this pull request as ready for review May 17, 2022 18:19
inmanturbo and others added 3 commits May 18, 2022 10:02
Column selection should be disabled for multiple of same component. 
- It has never worked and the only way to make it work now would be to pass in some identifier at mount() and use that to change the fingerprint with `setDataTableFingerprint()`. 
- Due to the high potential for errors in setting arbitrary properties and manipulating them at mount(), then attempting to use them in configure(), I think it's best not to support this type of configuration in the docs, although IT CAN BE DONE.
- There is a suggestion in the docs for reusing components on different pages which is less error prone.
@inmanturbo inmanturbo marked this pull request as draft May 18, 2022 16:23
@inmanturbo inmanturbo marked this pull request as ready for review May 20, 2022 14:52
@inmanturbo inmanturbo marked this pull request as draft May 26, 2022 19:31
@inmanturbo inmanturbo marked this pull request as ready for review May 26, 2022 19:56
@rubensrocha
Copy link

Does this PR fix this issue #760?

@inmanturbo
Copy link
Contributor Author

Does this PR fix this issue #760?

Yes it does. And it should be good to go. I just wanted Anthony to look over it before merging it in. I'm sure he'll review the open PR's before the next release, and he releases often. He just got sick and things got a little backed up for him. Should be back soon.

@rappasoft rappasoft merged commit 7d2a8c8 into develop Jun 8, 2022
@rubensrocha
Copy link

I look forward to the release of a new version to include this very important fix. :)

@rappasoft rappasoft deleted the column-select branch July 10, 2023 00:59
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.

4 participants