-
Notifications
You must be signed in to change notification settings - Fork 436
Add revision settings to Update Site #187
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
Conversation
How come you didn't added a getter / setter for revisionLimit? How come these values are not read from get requests? I haven't checked the code to see if we are returning them but it seems like we would be. |
I didn't change the model, it was already there so I didn't even check, I just added it to the serializer |
Huh. Not good that we have a partial implementation like that. Oh well. Thanks for fixing! For your CL, though, it looks like we are simply naming the member "revision_limit" and not using a getter / setter. This doesn't match any of our other properties. Can you fix that? We might also debate on requiring revision_history_enabled to be true to set it but for now, I don't think that matters. Besides that it looks great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 🎸 🇫🇷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rocket
Revision History settings were present in the SiteItem model but not actually serialized and didn't have setters/getters. I've updated that and enhanced the is_int decorator to allow exemptions in the case of sentinels that fall outside the normal range.
Addresses #186
(And helps me with some internal stuff)
Updated tests and manually tested internally