-
Notifications
You must be signed in to change notification settings - Fork 43
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
WIP: Cancel message #370
base: devel
Are you sure you want to change the base?
WIP: Cancel message #370
Conversation
@whirm I'll remove the pep8 commit for now, otherwise the PR is too difficult to review. |
9d574c6
to
4e5b7ef
Compare
Test FAILED. |
@LipuFei yes, those should always go alone |
@LipuFei why are you adding the cancel to the permission triplet? That shouldn't be neccesary. You authorise someone to send a dispersy_cancel message etc. |
@NielsZeilemaker I made this PR to make it easy for us to review. Thanks! I'll check everything again. |
@NielsZeilemaker What does the |
Basically, you can authorize/revoke or grant someone the permission to create a message of a certain type. I wouldn't start messing with this as its never tested/understood by me. |
Btw, the documentation regarding the permissions should still be correct. You can have a look there. |
@NielsZeilemaker I had thought about it and I decided to duplicate the undo stuff instead of messing them up. Later, when we want to remove the undo, we can simply remove every undo-related thing without worry about causing damage to the cancel message. I will read the permission docs. Thanks! |
for message in community.get_meta_messages(): | ||
# grant all permissions for messages that use LinearResolution or DynamicResolution | ||
if isinstance(message.resolution, (LinearResolution, DynamicResolution)): | ||
for allowed in (u"authorize", u"revoke", u"permit"): | ||
for allowed in (u"authorize", u"revoke", u"permit", u"cancel"): |
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.
I would remove the cancel here, but leave it in timeline.py. E.g. add it when the "undo" thingy is there.
4e5b7ef
to
d84e09b
Compare
Test FAILED. |
9d0f83b
to
b6cd526
Compare
Test FAILED. |
Test FAILED. |
@whirm I guess this is more or less it. I will check the unit tests one more time after my presentation rehearsal. Also, before we merge, we should change the AllChannel community to use this cancel message and test on jenkins. |
Ok, let's review it later |
b6cd526
to
162c809
Compare
Merged build triggered. |
Merged build started. |
Merged build finished. |
Test FAILed. |
Merged build triggered. |
Merged build started. |
Merged build finished. |
Test FAILed. |
Refer to this link for build results (access rights to CI server needed): |
@NielsZeilemaker One of the cancel message tests didn't pass. The test does the following:
This test was passing before. Am I doing it correctly? If a user's cancel permission has been revoked, should his old cancel messages be regarded as invalid as well? |
@LipuFei maybe create a wiki page describing the cancel message, and what it should do. Then afterwards, I can have a look and maybe we can modify the behaviour of your implementation accordingly. |
@NielsZeilemaker I've made a wiki page: https://github.com/Tribler/dispersy/wiki/Cancel-Message. Hope it explains. |
@LipuFei I made some changes to the document. I guess cancel messages should also not cancel an authorize message right? Or are we adding that? Additionally, we should think about being able to drop cancelled messages from the database in order to save space. |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
1ced941
to
475c87b
Compare
Refer to this link for build results (access rights to CI server needed): |
475c87b
to
1709e3c
Compare
Refer to this link for build results (access rights to CI server needed): |
Hi @NielsZeilemaker, could you review this PR again? Thanks! |
Use cancel message to replace undo message.
All undo message code remain the same.