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.

Sign up for GitHub

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

Jump to bottom

fix: add the operation name to the unpermittable column error #723

Merged
merged 2 commits into from
Sep 3, 2021
Merged

fix: add the operation name to the unpermittable column error #723

merged 2 commits into from
Sep 3, 2021

Conversation

RhnSharma
Copy link
Contributor

Fixes #719
Hi @jwoertink, how does this look?
Let me know if this needs any changes.
Thanks

jwoertink
jwoertink requested changes Aug 8, 2021
View reviewed changes
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Thanks for taking this on. I left a comment just saying that simply adding the type makes the error message a little confusing. I'd recommend giving it a shot to raise the error so you can see what I mean. After that update, it should be good 👍

src/avram/save_operation.cr Outdated
@@ -260,7 +260,7 @@ abstract class Avram::SaveOperation(T)
@@permitted_param_keys << "{{ attribute_name.id }}"
{% else %}
{% raise <<-ERROR
Can't permit '#{attribute_name}' because the column has not been defined on the model.
Can't permit '#{attribute_name}' because the column has not been defined on the model #{@type}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't quite right since @type isn't a model, it's the SaveOperation class. To test the error message, look for any SaveOperation in the specs, and add any random word to the permit_columns like banana or something. Then try to run the specs, and you'll see the error message.

This will need to say something more like defined on the model for SaveUser

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avram-SaveUser-error
@jwoertink Is this a valid error? Or do I need to make any changes? Could you please guide me a little?
Thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good error 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the changes. Please take a look at it and let me know if this needs any changes.
Thanks

@jwoertink
Copy link
Member

Hey @RhnSharma just wanted to ping you. There's just a small change to your PR, then we can merge it in when you get a free moment. Thanks!

jwoertink
jwoertink approved these changes Sep 3, 2021
View reviewed changes
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thank you for making this change.

@jwoertink jwoertink merged commit a3d830c into luckyframework:master Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwoertink jwoertink jwoertink approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the operation name to the unpermittable column error
2 participants
@RhnSharma @jwoertink