Fix bug when adding roles to a model that doesn't yet exist

[duncanmcclean]

Brief description of the bug
In our app, we have Nova setup so the client can create new users and assign them different roles within the application. We're using this package to bring the Role adding, etc functionality to Nova.

However, whenever a new user was created and roles were assigned to that user, it seemed to override the roles of other users as well.

Debugging
I spent an hour or so trying to get to the bottom of this. I did some debugging in the Nova package itself but couldn't find anything, then I started looking at this package.

When dd'ing (sorry not Ray just yet), that $object was an instance of one of the other models (a different user), whereas $model was the instance of the model I was creating in Nova.

I changed around references of $object to $model and hey-presto, it started to function as expected (eg. the roles for the new user were only assigned to the new user, not anyone else).

---

Let me know if you have any questions or if you'd like me to make any changes.

[drbyte]

@damcclean Thanks for this.

What are your thoughts on updating the test suite to match?
And perhaps we should apply the same change for permissions as well?

[duncanmcclean]

Hi,

I'm happy to update the tests. Which ones do you think would need updating?

Sure, I'll add the same to Permissions as well.

Hopefully I'll be able to look at it in the next couple of days.

[drbyte]

I wonder if these tests are incomplete, given that they didn't catch the situation you're reporting:

it_will_sync_roles_to_a_model_that_is_not_persisted()
calling_syncRoles_before_saving_object_doesnt_interfere_with_other_objects()
calling_assignRole_before_saving_object_doesnt_interfere_with_other_objects()
.. and their permission equivalents

[duncanmcclean]

@drbyte I've applied the same fix to the HasPermissions trait and I've updated the various tests to hopefully cover the issue.

[drbyte]

@damcclean Thanks.

In looking at the tests, I'm not certain these test changes are covering the situation you're trying to fix. Ideally we want the test to fail if the changes to the trait aren't made.

[drbyte]

@damcclean I've made a couple updates to your PR which I think solve this. By skipping the temporary static variable I believe the purpose it was attempting to solve is handled even better by swapping $model for $object in the remaining lines.

Can you test your app with this change?

[duncanmcclean]

Hi there,

Sorry for taking a long time to get back to you - I only work on this project once a week so I was trying to fit in changes when I'm on the project 🙂

We've just updated the vendor files on the project and we're able to verify the issue is resolved by them.