Fixed #13376: Added restrictions to messages to control when they are hidden.#18286
Fixed #13376: Added restrictions to messages to control when they are hidden.#18286nanorepublica wants to merge 3 commits intodjango:mainfrom
Conversation
There was a problem hiding this comment.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
|
This is my initial application of the last patch attached to the Trac ticket. There is more to do to apply the full patch, which I will be adding soon |
matthiask
left a comment
There was a problem hiding this comment.
Interesting, thanks. I want to pay attention to this because of https://pypi.org/project/django_user_messages/
| return ( | ||
| self.level == other.level | ||
| and self.message == other.message | ||
| and self.restrictions == other.restrictions |
There was a problem hiding this comment.
If restrictions were made hashable self.restrictions could be a set(), which would mean that this comparison isn't depending upon the ordering of restrictions in the messages themselves.
| def create_message(self, *vars): | ||
| """creates message on the basis of encoded data""" | ||
| args = vars[:-1] | ||
| restrictions = vars[-1] |
There was a problem hiding this comment.
I appreciate keyword-only arguments...! Makes sense, but it's not nice to look at.
|
@matthiask Thanks for the notes. This PR is a simple application of the patch that was originally on the ticket with most conflicts resolved. I am pausing on this PR until I get this smaller PR merged with does less in terms of code changes but allows third-party packages like yours to extend the messages framework in a more useful way which would include the restrictions mentioned here. Once that PR is merged I will reconsider this PR in the context of the ticket. |
| created = time_provider.time() | ||
| self.expires = created + int(seconds) | ||
|
|
||
| def set_expirity_time(self, expiration_time): |
There was a problem hiding this comment.
| def set_expirity_time(self, expiration_time): | |
| def set_expiry_time(self, expiration_time): |
|
|
||
| def set_expirity_time(self, expiration_time): | ||
| """ | ||
| Sets expilcity expiration time |
There was a problem hiding this comment.
| Sets expilcity expiration time | |
| Set expiration time to the given absolute value |
| def from_json_param(cls, expirity_time): | ||
| ret = TimeRestriction(0) | ||
| ret.set_expirity_time(expirity_time) |
There was a problem hiding this comment.
| def from_json_param(cls, expirity_time): | |
| ret = TimeRestriction(0) | |
| ret.set_expirity_time(expirity_time) | |
| def from_json_param(cls, expiry_time): | |
| ret = TimeRestriction(0) | |
| ret.set_expiry_time(expiry_time) |
|
|
||
| def __init__(self, amount): | ||
| assert int(amount) >= 0 | ||
| Restriction.__init__(self, "amount") |
There was a problem hiding this comment.
Should we use a call to super() instead of calling Restriction explicitly?
| self.level = int(level) | ||
| self.message = message | ||
| self.extra_tags = extra_tags | ||
| self.restrictions = restrictions or list([res.AmountRestriction(1)]) |
There was a problem hiding this comment.
Is the list() call necessary?
| """ | ||
|
|
||
| def __init__(self, level, message, extra_tags=None): | ||
| def __init__(self, level, message, extra_tags=None, restrictions=[]): |
There was a problem hiding this comment.
Would it be better to keep away from putting a mutable list as a default, and use None, in case it opens up to mutation bugs later?
Trac ticket number
ticket-13376
Branch description
This PR adds the ability to control when a message is hidden from view.
Currently there is restrictions for the number of times a message is iterated over and for an expiry time.
Checklist
mainbranch.