-
Notifications
You must be signed in to change notification settings - Fork 14
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
Adapt errors for active admin #44
Conversation
@juanmanuelramallo github actions are not running on forks, do you know how to solve it? |
I can make this pr from a local branch if it helps |
Thanks but don't worry I think we need to find a solution if we want to continue using Github Actions for OSS projects |
private | ||
|
||
def sanitize_errors | ||
form_attributes_validations = self.class.validators.map(&:attributes).flatten |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
form_attributes_validations = self.class.validators.map(&:attributes).flatten | |
form_attributes_validations = self.class.validators.flat_map(&:attributes) |
@andresg4 wouldn't this work? https://engineering.liefery.com/2019/07/03/form-objects-and-active-admin.html section |
|
||
def sanitize_errors | ||
form_attributes_validations = self.class.validators.map(&:attributes).flatten | ||
form_attributes_validations.each do |form_attributes_validation| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question, imagine I have the model User
with an attribute username
with a presence validation, and the form object UserForm
with a length validation on the username
. both errors will end up in user_form.errors[:username]
right? In that case we don't want to move it up to the base
because it already has a field were to be displayed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @santib, you're right about this. We should figure out another way to only move to base
the form object validations errors. Maybe running this sanitize_errors
before we move the model errors into the form, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andresg4 that will solve the model error to be displayed in the correct field, but the form validation shouldn't be moved either if it has a specific field where to be displayed. My thinking is that if an error is set in an attribute that will be displayed in the page we don't do anything, if it doesn't have a place in the page we move it to the base. The question is how can we know that 🤔
@santib Do you mean to run on a pull request? in that case modify the ci.yml and change the
There are other triggers here |
Great! I'll try that, thanks! |
Adapt errors for Active Admin
These changes try to solve #43.
The form object validations' errors are loaded to the
:base
key, so when addingf.semantic_errors
to the active admin form it will display them.Maybe it's worth a discussion if this is useful for the users or not. Let me know what you think.
In my experience, I had to manage the errors in the admin, with some if's and showing them with flash. This solution is simpler I think but maybe it doesn't worth it to add this changes since the users have to add some things on their end.
Thanks to @santib for all the help 🙌 .
For this to work correctly the user will have to add some changes.
include ::YAAF::ActiveAdmin
to the form object.f.semantic_errors
to the form object.self.main_model_class
in the form object, with the class of the model of the active admin view:accepts_nested_attributes_for
in the model is still needed, as active admin depends on it.To do: