Skip to content

Closes #17608: Adds L2VPN.status field #18791

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

Merged
merged 12 commits into from
Mar 6, 2025
Merged

Conversation

jnovinger
Copy link
Member

Fixes: #17608

  • adds L2VPN.status field with L2VPNStatusChoices as choices
  • adds migration for L2VPN.status
  • updates forms and filtersets for L2VPN.status
  • updates UI views (tables and detail template) and API serializer for L2VPN.status
  • updates L2VPNIndex to display status in results
  • fixes tests to use new L2VPN.status field

@jnovinger jnovinger marked this pull request as draft March 3, 2025 17:47
@jnovinger jnovinger marked this pull request as ready for review March 3, 2025 18:11
@jnovinger jnovinger requested review from a team and arthanson and removed request for a team March 3, 2025 18:11
@jnovinger
Copy link
Member Author

@arthanson , since you won the review lottery: any guidance on what types of fields should be added to a model's clone_fields attribute?

@jnovinger
Copy link
Member Author

jnovinger commented Mar 3, 2025

Doh, just realized I forgot to update the model docs. Will get that momentarily. Done

@jnovinger jnovinger changed the title Closes #17608: adds L2VPN.status field Closes #17608: Adds L2VPN.status field Mar 3, 2025
@arthanson
Copy link
Collaborator

@arthanson , since you won the review lottery: any guidance on what types of fields should be added to a model's clone_fields attribute?

There isn't a set rule - It's used for create and add another for defaulting fields when entering many items, so generally anything that isn't a unique constraint or you will probably change anyways (name, description, position). Status is generally included in cloned_fields, if not probably either a specific reason or an oversight.

Copy link
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

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

Looks good - should add a test to test_filtersets and maybe test_api as well as it is a required field so not just testing the default.

@jnovinger jnovinger force-pushed the 17608-add-L2VPN-status-field branch from a63b52d to 7bd708b Compare March 6, 2025 17:10
@jnovinger jnovinger requested a review from arthanson March 6, 2025 19:00
@arthanson arthanson merged commit 6bc9302 into feature Mar 6, 2025
6 checks passed
@jnovinger jnovinger deleted the 17608-add-L2VPN-status-field branch March 13, 2025 22:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants