search

Laravel-Backpack / PermissionManager

Admin interface for managing users, roles, permissions, using Backpack CRUD
http://backpackforlaravel.com
Other
516 stars 166 forks source link

[Bug] Tests fail possibly trying to load filters on non-PRO setup #345

Closed nhunt290 closed 9 months ago

nhunt290 commented 10 months ago

Bug report

What I did

I have a UserCrudController with the method list method

protected function setupListOperation()
    {
        CRUD::setFromDb(); // set columns from db columns.
    }

I created a test to check my user.index route is accessible

public function test_admin_can_see_user_list()
    {
        $user = User::factory()->admin()->create();

        $this->actingAs($user)
            ->get(route('user.index'))
            ->assertOk();
    }

What I expected to happen

I would expect the test to pass.

My test for the dashboard route passes just fine:

public function test_admin_can_access_admin_panel()
    {
        $user = User::factory()->admin()->create();

        $this->actingAs($user)
            ->get(route('backpack.dashboard'))
            ->assertSeeText('Logout')
            ->assertOk();
    }

What happened

Test fails with the following exception: Next Spatie\LaravelIgnition\Exceptions\ViewException: None of the views in the given array exist. in /Users/me/projects/b-f-l/vendor/laravel/framework/src/Illuminate/View/Factory.php:168

What I've already tried to fix it

The above exception comes from this method getting the first view that exists:

public function first(array $views, $data = [], $mergeData = [])
    {
        $view = Arr::first($views, function ($view) {
            return $this->exists($view);
        });

        if (! $view) {
            throw new InvalidArgumentException('None of the views in the given array exist.');
        }

        return $this->make($view, $data, $mergeData);
    }

I added dd($views) above the $view declaration and ran the test again which returned:

array:1 [
  0 => "crud::filters.dropdown"
]

Is this related to previous issues like the following? https://github.com/Laravel-Backpack/PageManager/issues/123 https://github.com/Laravel-Backpack/CRUD/issues/4196

For me, the last time I used backpack was v4 and I enjoyed it, so decided to use it again this time around. But after reading the above articles and checking the docs, it seems filters are now a PRO feature, which makes me think it's related the above issues in that the error message is not at all clear.

Just to try it, I created an empty vendor/backpack/crud/filters/dropdown.blade.php file. Ran the test again, and this time I dumped out:

array:1 [
  0 => "crud::filters.select2"
]

So, I did the same as above, created a blank select2.blade.php file, ran the test again, and this time it passed.

I don't really need filters, yet. I may do further down the line but not now, so I don't want to simply just upgrade for the sake of it. Would there be a reason my tests seem to be trying to implement the filters? When I visit the user list page in the browser, I get no errors or any indication of it trying to load filters, so not sure why my test would be.

If this is a bug within Backpack, is there any way I can disable filters so they don't attempt to show? I don't really want to have to commit a couple of blank files if I don't have to.

Is it a bug in the latest version of Backpack?

After I run composer update backpack/crud the bug... is it still there? Yes

Backpack, Laravel, PHP, DB version

When I run php artisan backpack:version the output is: 

### PHP VERSION:
PHP 8.1.23 (cli) (built: Aug 31 2023 19:14:41) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.23, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.23, Copyright (c), by Zend Technologies

### LARAVEL VERSION:
10.28.0.0

### BACKPACK PACKAGE VERSIONS:
backpack/basset: 1.2.1
backpack/crud: 6.3.2
backpack/generators: v4.0.2
backpack/permissionmanager: 7.1.1
backpack/theme-tabler: 1.1.1
welcome[bot] commented 10 months ago

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication channels:

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

-- Justin Case The Backpack Robot

nhunt290 commented 9 months ago

I will add that I already had a Route::crud('user', 'UserCrudController'); route in routes/backpack/custom.php and hadn't realised Laravel-Backpack/PermissionManager was using a route with the same name which overwrited mine.

As that's the case and the test I was running won't have been using my route, but PermissionManager's, I think this is more of a bug with PermissionManager rather than CRUD.

I am assuming backpack_pro() here is returning true when testing https://github.com/Laravel-Backpack/PermissionManager/blob/main/src/app/Http/Controllers/UserCrudController.php#L56C13-L56C27

Testing against my version of the UserCrudController which is a standard one created using the backpack CLI, the test passes without the need to create the empty filter views.

Are you able to move this issue to Laravel-Backpack/PermissionManager or should I recreate it on there?

welcome[bot] commented 9 months ago

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication mediums:

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

-- Justin Case The Backpack Robot

pxpm commented 9 months ago

Hello @nhunt290 thanks for the report and through explanation.

Indeed, I can confirm that backpack_pro() returns true when running unit tests.

The main reason behind it, is that we can't add backpack/pro as a dev-dependency, as that would prevent any free user of backpack/CRUD from installing the open-source core dev dependencies without having a backpack/pro license.

This is indeed a rather interesting use case, as the backpack/PermissionManager is also a free package, and we wrap the paid stuff in a backpack_pro() conditional so that free users can also use it, if they don't need the added features with pro.

I am not sure what's the best way to handle this, using the package works, testing the package without pro is broken, but I am not seeing easy way out.

Maybe an .env variable instead of returning true when running unit tests ? Something like TESTING_PRO=true that we can setup on our test environments when needed ?

I will bring this to my colleagues attention so that we can try to figure out a better solution.

I can't promise any deadlines, as I don't think this is high-priority. It's annoying indeed and prevents people from testing stuff while using the Permission Manager controllers.

Thanks again for the detailed report and reproduction steps, they were very helpful 🙏

nhunt290 commented 9 months ago

Hey @pxpm, thanks for the response.

It is indeed an interesting use case.

I wonder if maybe you could check inside the backpack_pro() conditional if the app is running unit tests with a simple if(App::runningUnitTests()) {return true}.

Perhaps with a combination of APP_ENV=local and/or APP_DEBUG=true in case someone can fake the response of App::runningUnitTests() and get the paid features for free, as I can't imagine anyone deploying to production with such env values.. I hope.

But, I understand it's not a high priority and that's OK, I'll leave it with you and the team to decide the best workaround :)

pxpm commented 9 months ago

Hey @nhunt290 that's what we are doing already: https://github.com/Laravel-Backpack/CRUD/blob/0e14e07c8449a20595b71ca70c3d6fdf628f3449/src/helpers.php#L395 and it's not the best solution as we just found out 🙃

You dont' get the paid features for free, we just don't test those inside backpack/crud package, but we need the backpack_pro() call inside our codebase to return true when we are running the backpack/crud tests. That's why maybe an .env variable for the testing environments is better than our current solution 👍

Cheers

karandatwani92 commented 9 months ago

Closing this issue as there has been no activity. Please feel free to reopen if needed or if you have additional information to provide. Thanks!