Skip to content
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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dhomeier
Copy link
Contributor

@dhomeier dhomeier commented Jun 24, 2024

This PR addresses an issue I believe to be a bug in update_events: the loop in

Spond/spond/spond.py

Lines 332 to 334 in bcb2b6d

for event in self.events:
if event["id"] == uid:
break

will fall through, if no event matching uid exists, and proceed to modify or overwrite whatever event was last in the loop.

This adds following commits:

  1. Raise an exception if no event for uid is found.
  2. Turning this instead into a feature and taking up the idea from Publishing Events on Spond #39, lets update_events in that case instead create a new event from the user-provided updates.
  3. adds an example for using this new functionality by creating updates from iCal-formatted files (essentially inverting ical.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 using base_event.update(event); this will change the behaviour somewhat, as this also includes keys that are not present in the original base_event.

I also had to modify the defaults in base_events a bit, since the original dict in assignedTasks 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) the owners and recipients entries. I don't know exactly what environment the example code in #39 was operating in, in any case could not find anything corresponding to self.client_account_id or self.group_id. So in the new script I am using whatever information can be extracted from s.get_person and s.get_groups – the latter in particular might be suboptimal, as it relies on being the contactPerson for the given group.
Comments and suggestions welcome!

@dhomeier dhomeier force-pushed the update_and_create_events branch from 3af68ab to 3474964 Compare June 24, 2024 23:39
@dhomeier dhomeier force-pushed the update_and_create_events branch from 73b4d8a to fc30d4f Compare June 25, 2024 00:45
@elliot-100
Copy link
Collaborator

elliot-100 commented Jun 25, 2024

I've never used update_event() but my IDE was flagging quite a few issues... thanks for looking at this. I will take a proper look soon, but I do think that base_event should be refactored out to a file.

Also, the event lookup shown in your code above should probably be refactored so it shares code with get_event(). I have outstanding #113 related to this, will get that done first.

@dhomeier
Copy link
Contributor Author

Thanks, yes, I probably would have moved base_event at least outside of the function scope as well – also in case you prefer to add a separate function for create_event. I suspect some parts of the interface have changed since the original introduction of update_event and the example from #39, so tried to keep the template to a minimum, but that was rather based on experiment.

or updates["owners"][0].get("id") is None
):
errmsg = '"owners" need to have a valid user id'
raise ValueError(errmsg)
Copy link
Contributor Author

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.

Copy link
Collaborator

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

Actually my error has confused things - as per discussion at #113, PR #135 was to change failed lookups to return KeyError, and I've now amended so it does.

Copy link
Contributor Author

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! ;-)

@elliot-100
Copy link
Collaborator

elliot-100 commented Jul 11, 2024

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.

@dhomeier
Copy link
Contributor Author

Yes, I could strip this to just the first commit, and then see how to best synchronise the follow-up with #143.

@elliot-100
Copy link
Collaborator

Yes, I could strip this to just the first commit, and then see how to best synchronise the follow-up with #143.

All the other commit in #143 does is reorder/reorganise the existing tests into classes, so there should be no issue there.

@elliot-100
Copy link
Collaborator

elliot-100 commented Oct 21, 2024

I've merged PR #156, so

Raise an exception if no event for uid is found.

is covered.

Reading this bit again:

if no event for uid is found ... turning this instead into a feature ... lets update_events in that case instead create a new event from the user-provided updates.

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.

create_new_event() (or similar) should be its own callable function, which does just that. It might need to re-use some code from update_event() - if so, the commonality should be factored out to another function.

@elliot-100 elliot-100 added bug Something isn't working enhancement New feature or request labels Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants