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

Improve test coverage for v4 #55

Open
wants to merge 19 commits into
base: v4
Choose a base branch
from

Conversation

ryanmitchell
Copy link
Member

@ryanmitchell ryanmitchell commented Jun 22, 2022

These should all pass except should fail to create a location with a duplicate slug which will fail. Also the scopeListFrontEnd tests are failing due to the change from using a JSON field to a location options model - you dont seem to have set up that relation on the model - so we cant use a whereHas() type approach - what did you plan for that?

Because we're using request rules, it does mean that any changes/data applied directly to the model doesnt get checked against those validations and that seems like an oversight? Devs are likely to directly manipulate data and we should make sure its valid... do you have any thoughts on how best to approach it?

@ryanmitchell ryanmitchell changed the title Improve test coverage for Locations Model Improve test coverage for v4 Jun 23, 2022
@sampoyigi
Copy link
Member

Thanks for your patience as i am only now getting to this... I didn't add a relation to avoid breaking existing implementations that relied on the options attribute, however I've now added a safe relation for location options,

tastyigniter/TastyIgniter@652cb6e

Because we're using request rules, it does mean that any changes/data applied directly to the model doesnt get checked against those validations and that seems like an oversight? Devs are likely to directly manipulate data and we should make sure its valid... do you have any thoughts on how best to approach it?

Developers can subscribe to an event on the request rules to validate location options.

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.

3 participants