-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fix update_events
for nonexistent IDs and support creation of new events
#134
base: main
Are you sure you want to change the base?
Conversation
3af68ab
to
3474964
Compare
73b4d8a
to
fc30d4f
Compare
I've never used Also, the event lookup shown in your code above should probably be refactored so it shares code with |
Thanks, yes, I probably would have moved |
or updates["owners"][0].get("id") is None | ||
): | ||
errmsg = '"owners" need to have a valid user id' | ||
raise ValueError(errmsg) |
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 suppose the second part should become a LookupError
in the spirit of #135; for the first part, which covers an empty or altogether missing list of owners, it is not as clear – could of course handle both exceptions separately.
More or less the same for recipients
below.
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.
Oh, I forgot I already argued for that myself! ;-)
There is quite a bit here - maybe it is worth creating a PR for the basic fix first? See also my #143 as it refactors event/group lookups elsewhere. |
Yes, I could strip this to just the first commit, and then see how to best synchronise the follow-up with #143. |
I've merged PR #156, so
is covered. Reading this bit again:
I don't think it is a good idea to use a failed lookup as a trigger to publish a new event. I guess you might have a use case for doing that, but I feel that's logic that belongs in your application, not in this package.
|
This PR addresses an issue I believe to be a bug in
update_events
: the loop inSpond/spond/spond.py
Lines 332 to 334 in bcb2b6d
will fall through, if no event matching
uid
exists, and proceed to modify or overwrite whateverevent
was last in the loop.This adds following commits:
uid
is found.update_events
in that case instead create a new event from the user-providedupdates
.updates
from iCal-formatted files (essentially invertingical.py
) and posting them to a Spond group.The first commit also simplifies the logic for updating the keys in
base_event
a bit by usingbase_event.update(event)
; this will change the behaviour somewhat, as this also includes keys that are not present in the originalbase_event
.I also had to modify the defaults in
base_events
a bit, since the original dict inassignedTasks
would raise an error on posting.Could still need some advice on the structure of
spond
's event format, and how to best set (defaults for) theowners
andrecipients
entries. I don't know exactly what environment the example code in #39 was operating in, in any case could not find anything corresponding toself.client_account_id
orself.group_id
. So in the new script I am using whatever information can be extracted froms.get_person
ands.get_groups
– the latter in particular might be suboptimal, as it relies on being thecontactPerson
for the given group.Comments and suggestions welcome!