-
Notifications
You must be signed in to change notification settings - Fork 11
Allow multiple teams and groups in clarification destinations #193
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
base: master
Are you sure you want to change the base?
Conversation
Instead of allowing only a single team or all teams to receive the reply to clarification (or an announcement from the admin or judges), allow multiple teams and/or groups to receive the reply/announcement. There are several use cases for this: 1) teams that are located in a specific area of the floor or a different room. Q:"Where are our printouts?" Reply: "There is a problem with the printer in your area, and it will be fixed shortly" 2) a group in a specific region: Q:"Why arent we getting balloons?" Reply to group: "The balloon printer in your region is out of paper and it is being fixed"
The description of the restrictions on from_team_id and to_team_ids and to_groups_ids was (is still?) ambiguous. Maybe still need better wording.
Indicate that the recipient list is the UNION of the to_team_ids and to_group_ids. Fix examples to use new properties. Fix up some awkward wording.
The filtering example happened to use to_team_id for clarifications as an example. This has to change to to_team_ids. Also changed the example to include multiple teams
Contest_API.md
Outdated
| id | ID | Identifier of the clarification. | ||
| from\_team\_id | ID ? | Identifier of the [team](#teams) sending this clarification request, `null` iff a clarification is sent by the jury. | ||
| to\_team\_ids | array of ID ? | Identifiers of the [team(s)](#teams) receiving this reply, `null` iff a reply to all teams or a request sent by a team. | ||
| to\_group\_ids | array of ID ? | Identifiers of the [ group(s)](#groups) receiving this reply, `null` iff a reply to all teams or a request sent by a team. |
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.
| to\_group\_ids | array of ID ? | Identifiers of the [ group(s)](#groups) receiving this reply, `null` iff a reply to all teams or a request sent by a team. | |
| to\_group\_ids | array of ID ? | Identifiers of the [group(s)](#groups) receiving this reply, `null` iff a reply to all teams or a request sent by a team. |
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.
Fixed.
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.
Should it be allowed to specify team and group IDs that overlap? That is, a team that's also already part of a group?
recipient by passing `to_team_id=X`. To filter on a `null` value, | ||
pass an empty string, i.e. `to_team_id=`. It must be possible to | ||
recipient by passing `to_team_ids=X,Y`. To filter on a `null` value, | ||
pass an empty string, i.e. `to_team_ids=`. It must be possible to |
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.
This suggests a real change: instead of just requiring filtering on properies with ID
type, we would now also require filtering on properties with type array of ID
.
That adds a bunch of extra complexity like: if you specify to_team_ids=X,Y
, do you want to match entries where to_team_ids
is exactly [X,Y]
, contains either X
or Y
, has [X,Y]
as a subset, or... I don't think we should go there, and simply change this example to something else than clarifications.
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.
The "OR" of all the to_team_ids (or to_group_ds) makes the most sense. eg. "Any of these teams"
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 don't think we should go there, and simply change this example to something else than clarifications.
I agree, we should not add the requirement that you allow filtering on any property of type array of ID
.
On a related note, when we say "any property with type ID
" do we mean to include properties of type ID ?
. We should clarify.
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.
Discussed at meeting. We will not introduce filter on arrays as part of this change, but might later.
"text":"May I ask a question?","time":"2017-06-25T11:59:27.543+01","contest_time":"1:59:27.543"}, | ||
{"id":"2","from_team_id":null,"to_team_id":"34","reply_to_id":"1","problem_id":null, | ||
{"id":"2","from_team_id":null,"to_team_ids":["34"],"reply_to_id":"1","problem_id":null, |
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.
Would be good to have an example where there are multiple team IDs and/or a group ID.
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.
Agree. Will update.
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.
Updated an example to include 3 team ids and a group id in a response.
I had modified the description to say: "The recipients of a clarification are the union of |
I think that's a better formulation that would be good to change. But it's not what I was trying to address. I meant the following: let's say group |
I think we discussed this on the call two weeks ago. I thought we decided that by specifying it as a "union" it specifically permits you have a team in the |
I would indeed argue that this is overspecifying, but I don't even think it's so unreasonable. Having overlap between the sets specified by |
recipient by passing `to_team_id=X`. To filter on a `null` value, | ||
pass an empty string, i.e. `to_team_id=`. It must be possible to | ||
recipient by passing `to_team_ids=X,Y`. To filter on a `null` value, | ||
pass an empty string, i.e. `to_team_ids=`. It must be possible to |
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 don't think we should go there, and simply change this example to something else than clarifications.
I agree, we should not add the requirement that you allow filtering on any property of type array of ID
.
On a related note, when we say "any property with type ID
" do we mean to include properties of type ID ?
. We should clarify.
Contest_API.md
Outdated
| id | ID | Identifier of the clarification. | ||
| from\_team\_id | ID ? | Identifier of the [team](#teams) sending this clarification request, `null` iff a clarification is sent by the jury. | ||
| to\_team\_ids | array of ID ? | Identifiers of the [team(s)](#teams) receiving this reply, `null` iff a reply to all teams or a request sent by a team. | ||
| to\_group\_ids | array of ID ? | Identifiers of the [ group(s)](#groups) receiving this reply, `null` iff a reply to all teams or a request sent by a team. |
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.
As specified right now, nulls in one (but not both) of to_team_ids
and to_group_ids
is either illegal (because it must be null
iff reply to all), (or it requires the non-null value to actually specify all teams), or it could be interpreted as the union of something and all, i.e. all. I don't think either of those are what we want.
It should be allowed to specify only to_team_ids
(i.e. having to_group_ids
as null
) and that should mean that only the teams listed will get the message. Similar for only to_group_ids
,
Removed extra space (cosmetic). Added a couple of team ID's and a Group ID to one of the clarification examples.
Made it clear, that from_team_id, to_team_ids AND to_group_ids must be null (or empty) to send to all teams. I think that makes it clear that if either to_team_ids or to_group_ids is non-null(empty), then the message is not sent to all teams, rather, only those teams
Change to_team_id to to_team_ids and add to_group_ids
Removed the
to_team_id
property from the clarifications object, and replaced it withto_team_ids
andto_groups_ids
. This allows a clarification response (or announcement) to be sent to multiple teams and/or groups and of course everyone.Added a description of how the new properties work.
Changed the clarification examples to reflect the new properties.
Changed Filtering example to use
to_team_ids
since it previously usedto_team_id
.This is a "breaking change" in that clients and servers that expect the
to_team_id
property for clarifications will no longer work properly until they are updated to use the new property(ies).