Conversation
thefloweringash
left a comment
thefloweringash
left a comment
There was a problem hiding this comment.
This introduces a new concept of fields that determine identity, and locks them in to exactly type and id. I don't think this is a problem for anything we do or plan to do. It fixes a real problem with our site. If this conflicts with something we decide to do later, we can revisit.
LGTM.
| deserialize_context.run_callback(ViewModel::Callbacks::Hook::BeforeValidate, viewmodel) | ||
| viewmodel.validate! | ||
| # Validate, unless this is a id-only reference to an existing model | ||
| if !reference_only? || viewmodel.new_model? |
There was a problem hiding this comment.
Nitpick: comment is duplicating code, but using if instead of unless. If that's easier to understand, consider doing the same for the code.
| if !reference_only? || viewmodel.new_model? | |
| unless reference_only? && !viewmodel.new_model? |
If it's worth a comment, consider adding a short description of why it is valuable to skip validations in some cases.
If the request is making no changes to this model, don't require the model to be valid. This permits edits to other models that refer to this model, when this model is invalid.
(Or your favorite explanation here).
There was a problem hiding this comment.
I like both the change and the improved comment
chrisandreae
force-pushed
the
reference-only-handling
branch
from
da579f0 to
4f36198
Compare
If a deserialization operation is only pointing to something, and making no assertions about it, then we don't need to demand that the referenced model be a valid ActiveRecord.
chrisandreae
force-pushed
the
reference-only-handling
branch
from
4f36198 to
2eea809
Compare
2 participants
If a deserialization operation is only pointing to something, and making no assertions about it, then we don't need to demand that the referenced model be a valid ActiveRecord.
Unless we want to admit migrations that mutate ids, there's by definition nothing for a migration of a pure reference to do.
This may change with #182, so we will want to make sure that that supports renaming in the case of pure references.