Fix "Invite to <activity>" shown even for activities that don't support sharing #625
Conversation
erilyth
changed the title
|
Looks nice. Check your code with pep8 and do a test with activities that do not have the field max_participants in the file activity.info. (This field was a recent addition, not all the activities has been updated yet) |
|
In the activitybundle, max_participants is initialized to 0, so currently the "Invite" option is not displayed if max_participants is not set in the activity.info file. Should this be displayed for activities which don't have max_participants set as well? |
|
Reviewed this and also @i5o's abandoned #434. Agree with @godiard that commit should describe tests with activities that do not have the field max_participants. Please check that you are handling the journal activity correctly; the code in #434 is quite different to yours. In my opinion the invite option should be hidden for activities that do not have max_participants set. This is also consistent with #434. Please add to the commit message; what was wrong, what is fixed, how it is fixed. how to test. Please also rebase your patches on this pull request to create one patch, and then force push. (Because merging two patches will make history harder to use for regression analysis.) |
|
In #434 , The journal hides the invite menu as well, and the same thing is being done here, so I think it should be fine. Ill add details to the commit message. |
|
Any updates on this? |
|
@erilyth, I know how you feel; I've several pull requests waiting for review and merge. The next 0.108 date milestone is 28th December. Thanks for the force push, I've reviewed the commit. I continue to agree with @godiard that you should test with activities that do not have the field max_participants. You may have done this, I'm not sure. The best place to put it is in the commit message, because that is what will be used later when someone reports a bug. Write something like "Tested with activity Name-99 which does not have field max_participants." My other comments above, you have already integrated. |
|
Ok, sure quozl :). I will add that to the commit message and force push it. |
| @@ -24,11 +24,13 @@ | |||
| from gi.repository import GObject | |||
| import dbus | |||
|
|
|||
|
|
|||
|
This looks good, I will test. I'd appreciate it though if you kept the summary (the 1st line) of your commit message less thatn 80chars. Then it displays correctly in most places. Maybe like "Do not show invite option for activities that do not support sharing" (if that is less than 80chars @quozl My tests lead me to believe that this patch makes activities without max_partcipants not show the invite menu. I agree that this is a good thing, as older activities collab is probably broken anyway (from my fedora rawhide centric view of the world). I will merge once you fix up the commit message @erilyth! |
|
Done :) |
|
I continue to wish for the commit message to refer to the testing that has been done, rather than depend on GitHub to keep the pull request discussion. At the moment the commit message does not do that. |
Activities with max_participants<=1 also have an invite option, now they don't. Now, we check for activities with less than 1 participants and disable the option. To test launch activity with max_participants<=1 or max_participants is not set, and the invite option will not be present. Tested this with the read activity which supports a max of 10 participants and with the write activity which has not specified max_participants.
|
Updated the commit message, added test details for the write and read activities. |
|
Thanks. I've no further wishes. |
|
Great! It still works as intended. Sorry for taking soo long to merge this. |
A fix for the issue #2702 (https://bugs.sugarlabs.org/ticket/2702)