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

UI allows creation of multiple RelationshipAssociations even for "one_to_*" relationships #332

Closed
mzbroch opened this issue Apr 16, 2021 · 3 comments · Fixed by #350
Closed
Assignees
Labels
type: bug Something isn't working as expected
Milestone

Comments

@mzbroch
Copy link
Contributor

mzbroch commented Apr 16, 2021

Environment

  • Python version:
  • Nautobot version: develop branch os of Apr-16,

Steps to Reproduce

  1. Create a custom relationship (one to many) between Circuit and IP
  2. Assign the same IP to two different Circuits

Expected Behavior

Under IP Address view, relationship should indicate multiple circuits.

Observed Behavior

IP Address view shows only one related Circuit

image

image

@glennmatthews
Copy link
Contributor

This seems like a validation error rather than a UI issue - for a ONE_TO_MANY relationship, you shouldn't be able to assign a "many" object to two "one" objects simultaneously.

@mzbroch
Copy link
Contributor Author

mzbroch commented Apr 16, 2021

In this example one to many reflects modelling of a single circuit with multiple IP addresses related - I understand an IP address could be re-used by many Circuits. (no matter if it is real-life situation or not)

@glennmatthews
Copy link
Contributor

If you want to allow a single circuit to use multiple IPs and a single IP to use multiple circuits, that would be a MANY_TO_MANY relationship, not a ONE_TO_MANY relationship, as I understand things anyway. In any case, there's definitely an issue here, and thank you for the report!

@glennmatthews glennmatthews added the type: bug Something isn't working as expected label Apr 16, 2021
@glennmatthews glennmatthews self-assigned this Apr 19, 2021
@glennmatthews glennmatthews changed the title Custom Relationship UI issues UI allows creation of multiple RelationshipAssociations even for "one_to_*" relationships Apr 19, 2021
@paulbukauskas paulbukauskas added this to the v1.0.0 milestone Apr 20, 2021
@glennmatthews glennmatthews added this to To do in Release v1.0.0 via automation Apr 20, 2021
@glennmatthews glennmatthews moved this from To do to In progress in Release v1.0.0 Apr 20, 2021
@glennmatthews glennmatthews moved this from In progress to Review in progress in Release v1.0.0 Apr 20, 2021
Release v1.0.0 automation moved this from Review in progress to Done Apr 21, 2021
jathanism added a commit that referenced this issue Apr 21, 2021
- `PrefixFieldMixin` and `AddressFieldMixin` have `save()` methods that override any provided `commit=True` value, always calling `super().save(commit=False)`. `RelationshipModelForm` only updates `RelationshipAssociations` when called with `save(commit=True)`. As a result, when editing an IPAddress, Prefix, or Aggregate in the UI, any changes made to relationship associations would not take effect. I've fixed this by changing the subclass inheritance order for these forms so that the `PrefixFieldMixin`/`AddressFieldMixin` comes **after** the `RelationshipModelForm` so that the method resolution order is such that the `RelationshipModelForm` gets the intended `commit` value and can correctly create/update/delete `RelationshipAssociation` records as needed.
- The `render_form` template was missing logic to exclude relationship fields from the rendered form. As a result, many UI edit views would show relationship fields twice - once in the main rendered form, then again in the separate Relationships form panel. I've added the missing logic.
- To fix #332, I've implemented a `clean()` method for `RelationshipModelForm` that checks whether any requested relationship associations would violate a `ONE_TO_*` cardinality constraint on the relationship. I've also added a `test_forms.py` test script to exercise this code.
- I also addressed the FIXME in the `RelationshipModelForm` `_save_relationships()` to call `RelationshipAssociation.clean()` before saving it, though I have not thus far encountered any particular cases that trigger issues with the model clean.
- To avoid ambiguity in the UI (and fix #333), `*_TO_MANY` relationships now display the name of the related object type as well as the count (e.g., "2 VLAN Groups", instead of the previously displayed bare "2")
- Housekeeping item: in implementing `test_forms.py` I discovered that the relationship form field naming was "backwards", i.e. the form field for a relationship where the local record is the "source" and the form field lets you specify the "destination" was named `cr_my-relationship__source` where `cr_my-relationship__destination` would be more accurate. I've fixed this.
- Changed the `Relationship.__str__()` method so that it preserves existing capitalization of the relationship name

Co-authored-by: Jathan McCollum <jathan@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug Something isn't working as expected
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants