-
Notifications
You must be signed in to change notification settings - Fork 29
Fix update_events for nonexistent IDs and support creation of new events
#134
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: 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
uidexists, and proceed to modify or overwrite whatevereventwas last in the loop.This adds following commits:
uidis found.update_eventsin that case instead create a new event from the user-providedupdates.updatesfrom 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_eventa 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_eventsa bit, since the original dict inassignedTaskswould 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) theownersandrecipientsentries. 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_idorself.group_id. So in the new script I am using whatever information can be extracted froms.get_personands.get_groups– the latter in particular might be suboptimal, as it relies on being thecontactPersonfor the given group.Comments and suggestions welcome!