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

Fix LocationForm.parent auto-fill (again), by revamping prepare_value yet again #3412

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

glennmatthews
Copy link
Contributor

Closes: #3347

What's Changed

It turned out that #2773, in trying to fix #2470, did partially regress #2311, resulting in the filing of #3347. Along the way, fixing this intersected with code introduced way back in #553 to address #512, resulting in me needing to rework that bit of code as well. Then manual testing to verify that I've fixed #3347/#2311 without reintroducing #2470 or #512. Wheeeee!

  • The root of the issue in Cloning Device or IP with a tag applied results in a Server Error #512 was that normalize_querydict could only guess whether a given query parameter should be recorded as a single value or a list of values based on how many times it appeared in the query, meaning that a list parameter that was only used once in a given query would be incorrectly treated as a single value rather than a list with one entry. This would then cause problems in prepopulating an object form based on this incorrect treatment.

    • Solution then: add a special case in DynamicModelMultipleChoiceField.prepare_value to convert a single string value into a list value containing that single string.
      • The problem with this was, because of some Django internals involving rendering the individual <option> elements for the form field, that when provided a single object value (a model instance), it needed to stay as that single value, and not be converted to a list of a single object.
    • Solution now: remove the former code, and instead add an optional form_class parameter to normalize_querydict and update model views to provide this parameter as appropriate; when present, this parameter is used to check what form field a given query parameter corresponds to, and if the field is a multiple-choice field, correctly convert the single-valued parameter to a single-entry list.
    • This did require moving some code around in extras to support the JobView case, where the form class is generated on-the-fly from the selected Job, to make sure the form class was available at the time that the querydict is interpreted. This should have no functional impact.
  • The underlying issue with Editing a Location doesn't retrieve parent #2311 and Child location's slug is autogenerated using parent's UUID rather than slug #2470 "feels" like a latent Django bug to me but I'm probably missing something. Basically what it boils down to is that with a baseline ModelChoiceField that defines a to_field_name value:

    • prepare_value(object_instance) returns the value of the instance's field corresponding to the to_field_name specified (or the instance's PK if no to_field_name), which correctly allows the rendered form to pre-select the appropriate <option value="field_value"> in the HTML.
    • but prepare_value(object_pk) simply returns the provided PK,, regardless of any to_field_name, which doesn't match up with any <option value="field_value"> in the HTML, resulting in no pre-selection in the rendered form.
    • So it appears that the fix here is to subclass the prepare_value() method and ensure that, when given a PK, it looks up the actual object instance (if any) and passes that instance through to the superclass for processing, ensuring correct form rendering in this case as well.

Cases manually tested:

  • Look for regression of Cloning Device or IP with a tag applied results in a Server Error #512 by pressing the Clone button on object instances with zero, exactly one, or more than one Tag applied, (in other words, zero, one, and more than one occurrence of tags= in the GET query parameters) and verifying that in all three cases the form is correctly prepopulated with the appropriate tag selection:

image

image

image

image

image

  • Hitting the Add child button on an existing location and confirming that the parent field is correctly populated and that slug autogeneration in the UI works correctly (this is functionally quite similar to the "Clone" case but with far fewer autopopulated selections)

image

I have not spent time writing unit or integration test automation for the above cases because much of this functionality will likely change drastically in 2.0 rendering such tests irrelevant.

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • n/a Documentation Updates (when adding/changing features)
  • n/a Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design

Copy link
Member

@bryanculver bryanculver left a comment

Choose a reason for hiding this comment

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

This is so meta it hurts my brain.

@bryanculver bryanculver changed the title Fix #3347 by revamping prepare_value yet again Fix Location.parent auto fill (again), by revamping prepare_value yet again Mar 10, 2023
@bryanculver bryanculver changed the title Fix Location.parent auto fill (again), by revamping prepare_value yet again Fix Location.parent auto-fill (again), by revamping prepare_value yet again Mar 10, 2023
@bryanculver bryanculver changed the title Fix Location.parent auto-fill (again), by revamping prepare_value yet again Fix LocationForm.parent auto-fill (again), by revamping prepare_value yet again Mar 10, 2023
@glennmatthews glennmatthews merged commit 372d01e into develop Mar 13, 2023
@glennmatthews glennmatthews deleted the u/glennmatthews-3347-fun branch March 13, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants