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

feat: add possibility to disable the trait programmatically #43

Merged
merged 4 commits into from
Sep 7, 2022
Merged

feat: add possibility to disable the trait programmatically #43

merged 4 commits into from
Sep 7, 2022

Conversation

simbig
Copy link
Contributor

@simbig simbig commented Aug 31, 2022

Hello!

I have added the possibility to disable the property programmatically.

Usecase:
In the local environment I want to run seeders and factories and create a model. Only when the environment is set to production, I want the model to be read-only.

Code on the model

protected static function isActive(): bool
    {
        return 'production' === app()->environment();
    }

Unfortunately, I did not grasp the kahlan/kahlan-test package and thus did not add any tests yet.
@michaelachrisco Any idea how to test this feature?

@michaelachrisco
Copy link
Owner

@simbig Looks nice! Kinda like rubys syntax on models. The "instructions" to test probably should be on the README, ive just never had anyone ask about it.

You could use the same steps outlined in the CI/CD job here: https://github.com/michaelachrisco/ReadOnlyTraitLaravel/blob/main/.circleci/config.yml and re-run locally to test. I would like to have at least one test associated with the new isActive functionality, if you would be so kind.

@michaelachrisco
Copy link
Owner

michaelachrisco commented Aug 31, 2022

@simbig I added the ability to Build forked pull requests on Circleci, if you push to this repo again, the test should fire up. Hope that helps!

@simbig
Copy link
Contributor Author

simbig commented Sep 1, 2022

@michaelachrisco the problem is that not throwing the exception will actually proceed and hit the DB. As there is not DB and app installed with the trait I am not sure how to perform any assertions. (see new commit)

@michaelachrisco
Copy link
Owner

michaelachrisco commented Sep 1, 2022

From what I see we have a couple of options:

    it("stubs a function", function() {

        allow('time')->toBeCalled()->andReturn(123);
        $user = new User();
        expect($user->save())->toBe(true)
        expect($user->created)->toBe(123);

    });

    it("stubs a class", function() {

        allow('PDO')->toReceive('prepare', 'fetchAll')->andReturn([['name' => 'bob']]);
        $user = new User();
        expect($user->all())->toBe([['name' => 'bob']]);

    });

You can stub the function or class to see if the method on the model was called. You also have the option of just accepting that a Closure is returned. Both are "correct" in my opinion as it goes down the happy and sad path respectively.

@michaelachrisco
Copy link
Owner

@simbig I added a simple ugly test that mocks the DB model instead of having it go through the DB layer. Ill be honest, its not my best work, but it does test the functionality we are going for. If you can find a better way of doing this, let me know. 😄 .

@clanofartisans
Copy link

I literally just came to check the readme to see if this was possible... Pleased to see that my timing is so good and this may be an option soon! I want to have a special command line option that updates a model's table from an API but otherwise have the Eloquent models remain read only in every other context. This seems like a pretty great solution for me!

@simbig
Copy link
Contributor Author

simbig commented Sep 5, 2022

@michaelachrisco your test seems to be a bit unconventional but tests the functionality. 👍 From my side we can use it like your suggested.

@clanofartisans Nice to see that you also need that feature. ;-)

@michaelachrisco
Copy link
Owner

Im going to merge. I know I could probably do a better, less code smelly test at some point...but it honestly doesn't need to be perfect in order to be useful. Unless we see immediate breakages, it should be "good enough" for our purposes.

Thanks @simbig for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants