Can add new role, edit name/description

[landjea]

So I added a new role, and was able to edit the name/description on it but if I try to modify an action permission on either my new role or the existing "Members" role, I get an sql error about a missing field value. "ownership_key". I can see that the keys all exist in the permissions table.

SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'ownership_key' cannot be null (SQL: insert into `permissions` (`ownership_key`, `action_key`, `role_key`, `entity_key`, `created_by`, `updated_at`, `created_at`) values (, edit, test, assignment, 1, 2015-09-30 13:09:58, 2015-09-30 13:09:58))

Again with that same fresh install from yesterday. My only change was to update composer.json to the "~0.1.0" version of laravel-governor as per other issue.

I took a look at your included RolesController@update, which is throwing the error on $currentPermission->save();, and when I spit out the variables which showed $ownership = 'view any' and $currentOwnership = null.

So it doesn't seem to be finding the $ownership using $currentOwnership = $allOwnerships->find($ownership); on line 109 of RolesController.

Also, I don't fully understand your if statement in your foreach.

foreach ($actions as $action => $ownership) {
                    if ('no' !== $ownership) {

I have never seen it written like that before. At first I thought you had written the if check backwards (string to variable whereas I am always used to variable to string) but now I am thinking ... is that inverse writing the equivalent of a str_search? Is it a way to say: if $ownership does not contain the characters 'no' ?

[mikebronner]

The form of the "if statement" you reference above is called a Yoda Condition: https://en.wikipedia.org/wiki/Yoda_conditions. It still performs the same check as the other way around.

Please check the version that composer actually installed. This issue was fixed in version 0.1.7. In composer.lock you should see an entry like this:

            "name": "genealabs/laravel-governor",
            "version": "0.1.8",

(Did you remember to run composer update after updating the version number in composer.json? Just asking because sometimes we forget silly things.)

If that is the case, you probably need to add the created_by column to all tables (run the example migration at the bottom of the README, after removing the created_by columns already added in some of the tables).

Let me know how that works. I was able to change abilities on roles in your project with those things in place. :)

[landjea]

Yep, I do silly stuff all the time. :smile:

Thanks for the Yoda conditions info. I hadn't come across that before so it was interesting.

Checked composer.json and it is

"require": {
        "php": ">=5.5.9",
        "laravel/framework": "5.1.*",
        "genealabs/laravel-governor": "0.1.8",
        "laravelcollective/html": "5.1.*"
    },

And each table has a createdby field (I ran the migration yesterday)_. Each createdby field had a NULL value (except for the one role I created which was a 1 for my userid)_

Thinking that might be the problem (all the NULL values), I changed the "update all tables" migration to just delete any created_by fields it found, instead of throwing an exception, and had it recreate all the createdby fields on all tables. (I realize that this would be bad and normally I would want to preserve that createdby information on a live site, but there is nothing of value here so kill/rebuild is fine for me)

C:\lamp9\www\smarch   30/09/2015  9:57:25.50
λ composer update
> php artisan clear-compiled
Loading composer repositories with package information
Updating dependencies (including require-dev)
Nothing to install or update
Writing lock file
Generating autoload files
> php artisan optimize
Generating optimized class loader

C:\lamp9\www\smarch   30/09/2015 10:01:28.20
λ php artisan migrate
Migrated: 2014_10_12_100000_update_tables_for_governor

C:\lamp9\www\smarch   30/09/2015 10:18:13.53

Still getting the SQL error. Not sure what else it could be considering you downloaded my repo and are using it....maybe it's my user info...I dunno. I guess I could create a new user and make him superadmin to see but at a loss.

Thanks.

[mikebronner]

check your composer.lock file, not the composer.json :)

[landjea]

Oh yeah. Forgot to check the lock file.

Dang. Got excited for a minute there.

{
            "name": "genealabs/laravel-governor",
            "version": "0.1.8",
            "source": {
                "type": "git",
                "url": "https://github.com/GeneaLabs/laravel-governor.git",
                "reference": "8448efcec3979f7f2f794160180e3ea6861b3987"
            },
            "dist": {
                "type": "zip",
                "url": "https://api.github.com/repos/GeneaLabs/laravel-governor/zipball/8448efcec3979f7f2f794160180e3ea6861b3987",
                "reference": "8448efcec3979f7f2f794160180e3ea6861b3987",
                "shasum": ""
            },
            "require": {
                "genealabs/bones-macros": "^0.2.33",
                "illuminate/support": "^5.1",
                "laravelcollective/html": "^5.1",
                "php": ">=5.5.9"
            },
            "require-dev": {
                "fzaninotto/faker": "~1.4",
                "mockery/mockery": "0.9.*",
                "orchestra/testbench": "~3.1",
                "phpunit/phpunit": "~4.0"
            },
            "type": "library",
            "autoload": {
                "classmap": [
                    "database/migrations",
                    "database/seeds"
                ],
                "psr-4": {
                    "GeneaLabs\\LaravelGovernor\\": "src/",
                    "GeneaLabs\\LaravelGovernor\\Tests\\": "tests/"
                }
            },
            "notification-url": "https://packagist.org/downloads/",
            "license": [
                "MIT"
            ],
            "authors": [
                {
                    "name": "GeneaLabs, LLC",
                    "email": "hello@genealabs.com"
                }
            ],
            "description": "Managing policy and control in Laravel.",
            "time": "2015-09-29 21:35:53"
        }

Also, as I am no doubt you are aware, making a second user and giving them SuperAdmin in the database equals the same SQL error.

Gonna throw this repo out, download again and composer update again.

[mikebronner]

Did you republish the assets after updating? I forgot to mention that. That will fix the erroneous form fields.

[landjea]

Nope. Did it. Nope still SQLerror.

C:\lamp9\www\smarch   30/09/2015 10:24:11.55
λ php artisan vendor:publish --tag=genealabs-laravel-governor --force
Copied File [\vendor\genealabs\laravel-governor\config\config.php] To [\config\genealabs-laravel-governor.php]
Copied Directory [\vendor\genealabs\laravel-governor\public] To [\public\genealabs-laravel-governor]
Publishing complete for tag [genealabs-laravel-governor]!

Rats. :smile: Was hoping it might be that.

[mikebronner]

Clear cache, both in artisan and browser?

[landjea]

So downloading the same repo, and running composer update again to get governor up to 0.1.8 and running migrations again seems to have it gotten to work this time. (WTH?)

I didn't delete the other repo, just cloned into another folder, so I am going to do a diff to see what, if anything, is different.

The only difference in steps that I did this time (that I can remember :confused: ) between this working repo and the broken one is that I updated all the tables for governor before running the db seed. But the tables seem identical too. I'll let you know if anything turns up.

[landjea]

Other than log files there is only two differences between the repos. Composer.json and composer.lock.

The lock file has a different hash (makes sense since the file is in a different path and/or the composer.json is different)

Composer.json in NON-working version

"genealabs/laravel-governor": "0.1.8",

Composer.json in working version

"genealabs/laravel-governor": "~0.1.8",

But since the lock files are identical I don't think it is that as they are both using the same version. I believe that it must have been the database that was borked somehow. I have a copy of the borked DB at home (haven't messed with that one so it will be like I started today) so I will 1) check that it is broken and 2) compare that db to this db to see what the difference is that caused it to freak out like that.

It "feels" like it has to do with me running the db_seed after updating all the tables for governor.

Thanks for your assistance.

[mikebronner]

Yes, you're right, the seed should be run last, after migrating the tables and running the migration for the created_by columns. I will update the documentation to be more clear on that.

[landjea]

Yeah, when reading it, that makes perfect sense but I was just blindly copy/pasting from the install instructions and did the migration to "createdby" last each time. facepalm_

I am guessing you have already considered it but, just in case, why not use a table to store the created by info instead of manipulating existing tables?

Something like :

table_key	entry_key	created_by
roles	2	1
posts	23	46
permissions	11	1

[mikebronner]

Yea, the reason I don't create a separate table is because the created_by data is intrinsic to each record on each table. This would create join tables and require additional relationships that would cause a lot of SQL queries and php overhead. Using the created_by column in each table is much more efficient and semantically clearer. But you are correct, from an installation point of view, your solution would be easier. :)

[landjea]

Fair enough. Was curious.

Are you planning on adding a "specific" option to the ownerships instead of just "any"? Or is that too granular?

For example: "SupportMods" role can "create/edit self posts" with "any forums" "SupportMods" role can "create/edit other posts" with "specific forum:4(SupportForum)" "BillingMods" role can "create/edit other posts" with "specific forum:2(BillingForum)" "BillingMods" role can "create/edit self posts" with "any forums"

That kind of thing. Having different access for a subset of the Entity.

EDIT : updated to add billing mods for clearer example.

[mikebronner]

I was thinking about that briefly, but that seems so very granular. However, I do have a use-case for it as well, just haven't tackled it yet. I think that's something I might look at down the road after Spark integration is done.

This would be something I would put in its own table. Basically this would allow users (not just admins) to select the visibility they want on items they create.

[landjea]

Yeah, I was thinking that the granularity could likely be accomplished with your current system but it would probably end up with a metric crapton of roles and permissions to get it there. :)

(FYI - I don't have a pressing need or a use-case for it, it was just a thought that came to me)

On Wed, Sep 30, 2015 at 3:48 PM, Mike Bronner notifications@github.com wrote:

I was thinking about that briefly, but that seems so very granular. However, I do have a use-case for it as well, just haven't tackled it yet. I think that's something I might look at down the road after Spark integration is done.

— Reply to this email directly or view it on GitHub https://github.com/GeneaLabs/laravel-governor/issues/9#issuecomment-144519529 .

[mikebronner]

Closing this for now, as the original issue seems resolved. If it does return, please open a new issue (this one got a bit sidetracked) :)