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

Extend ObjectChange change_context_detail to 400 chars #2602

Merged
merged 4 commits into from
Oct 17, 2022

Conversation

chadell
Copy link
Contributor

@chadell chadell commented Oct 11, 2022

Closes: nautobot/nautobot-app-circuit-maintenance#218

What's Changed

  • The change_context_detail added in version 1.4.1 was limited to 100 chars
  • Some applications have very long names (>100), as the one referenced in the issue: plugins-nautobot_circuit_maintenance-handle_notifications-handler-handlecircuitmaintenancenotifications
  • The fix increases change_context_detail from 100 to 400 chars

TODO

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

@chadell
Copy link
Contributor Author

chadell commented Oct 11, 2022

Adding @gsnider2195 as the original code contributor

@jvanderaa
Copy link
Contributor

Confirms to fix nautobot/nautobot-app-circuit-maintenance#218

@glennmatthews
Copy link
Contributor

Can we also make this more robust in general so that we don't hit this same issue in the future if (heaven forbid) we somehow end up with a plugin with a 200-character viewname? It'd be good to ensure that we automatically truncate the change_context_detail to fit, e.g. here at least:

https://github.com/nautobot/nautobot/blob/develop/nautobot/extras/signals.py#L83

@glennmatthews glennmatthews self-requested a review October 11, 2022 12:41
Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

Come to think of it, we have a similar potential issue with object_repr - it would be good to add a length check and automatic truncation here as well.

@@ -0,0 +1 @@
`ObjectChange.change_context_detail` extended from 100 to 200 chars
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`ObjectChange.change_context_detail` extended from 100 to 200 chars
Increased size of `ObjectChange.change_context_detail` field from 100 to 200 chars.

@glennmatthews
Copy link
Contributor

Ah - this is actually coming from https://github.com/nautobot/nautobot/blob/develop/nautobot/extras/jobs.py#L1243. Since Jobs can have a slug up to 320 characters in the worst case, we definitely need some truncation logic and/or to increase the limit to more than 200 chars.

@chadell
Copy link
Contributor Author

chadell commented Oct 11, 2022

https://github.com/nautobot/nautobot/blob/develop/nautobot/extras/jobs.py#L1243

I agree on the idea. I would cover the object_repr too.
I believe that the truncation is the most conservative approach, as it will cover our backs whatever we find in the future.
If there are no concerns, I would apply a 200 to both attributes, with truncation.

@jvanderaa
Copy link
Contributor

@chadell can we move to a 320 size on the context to match that of a slug?

@chadell
Copy link
Contributor Author

chadell commented Oct 11, 2022

We could move to 400 (to account for the application name).
But, I will also apply truncation to solve this once and for all.

@bryanculver bryanculver marked this pull request as draft October 11, 2022 19:57
@bryanculver
Copy link
Member

@chadell Moved to draft to indicate you're still updating the PR to make it visually distinct.

Please select "Ready for review" once the PR has been updated.

@chadell chadell self-assigned this Oct 13, 2022
…to the object_repr attribute. Both, for the ObjectChange model
Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

Looks functionally solid to me. Can we define and use some constants in nautobot.extras.constants for these two values so that we don't have to have hard-coded 200 and 400 in various places?

@chadell chadell marked this pull request as ready for review October 13, 2022 13:17
@bryanculver bryanculver changed the title Extend ObjectChange change_context_detail to 200 chars Extend ObjectChange change_context_detail to 400 chars Oct 17, 2022
@bryanculver bryanculver mentioned this pull request Oct 17, 2022
5 tasks
@bryanculver bryanculver merged commit ab58764 into develop Oct 17, 2022
@bryanculver bryanculver deleted the extend-change_context_detail branch November 15, 2022 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Email Sync - Unable to Create Raw Notification
4 participants