-
Notifications
You must be signed in to change notification settings - Fork 29
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
Use static GUIDs in the frab schedule, when possible #614
Conversation
@johnjohndoe: This probably doesn't help EventFahrplan, it looks like you use the |
EventFahrplan does not support guid yet. See: EventFahrplan/EventFahrplan#319 |
@stefanor It is legal to schedule the same page into multiple schedule items (e.g. we do this in PyConZA for lunch and a few other recurring things), so the page |
@saerdnaer, I believe you could share your experience here because you dealt with similar topics in the CCC context. |
Thanks, switching SReview to the GUID attribute should be easy (and it doesn't assume that the upstream GUID is an integer, so that's fine). That just leaves the slug, which I was also parsing from the conf_url attribute. It's not as critical; I can slugify myself if needs be, it's just that it's nicer if my slug and the upstream schedule's slug are the same. FWIW, the "debconf" script isn't actually being used anymore, the actual script that parses schedules these days is https://salsa.debian.org/debconf-video-team/sreview/-/blob/main/scripts/sreview-import which uses I should probably throw those old schedule parsers away by now. |
Whoops, actually forgot one thing. SReview's schedule parser also assumes that if the It would be nice if there were an explicit attribute to state the type of event, so I can filter on that. |
and actually the slug is there, I'm blind :) sorry. |
That's handled, by using the scheduleitem's PK when a page is scheduled more than once. |
Yeah, I was thinking about that, too. With the current implementation in wafer, the |
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 this is probably more useful for schedules than the current situation, so I'm approving.
Ideally we should have some tests for these, but since nothing is failing I assume we didn't have tests before either.
Perhaps a cleaner solution would be to have ScheduleItems be more firmly attached to the talk or pages they are connected to, so that ScheduleItems are not created and deleted all the time while modifying the schedule but only moved around.
Yeah, would probably be good to test.
That sounds like a pretty good idea. Make the ScheduleItem's slot be optional, so that we can keep it alive (but dormant) when something is unscheduled. |
Yeah, I had noticed that, and am selecting on that for now. It works for debconf21, which is what I care about in the immediate short term, but having a more "proper" solution where we also figure out that something isn't being recorded and therefore we don't need to care about it would be useful down the line. |
SReview is currently having to use the integer in the talk URL to track talks, as they move around in the schedule. There should be a better way.
https://salsa.debian.org/debconf-video-team/sreview/-/blob/main/scripts/util/schedule_parsers/debconf#L82
The frab XSD defines
<event>
'sid
attribute as being an integer, in a single namespace. To avoid duplicates (because Pages and Talks are separate DB entities in wafer, with their own ID space), we've used theScheduleItem
's ID in the past. But these change when a talk gets rescheduled.So, let's put more information into the GUID. There's enough room here to namespace kinds of schedule items from each other.
I could also imagine putting Talks, Pages, and other ScheduleItems in separate integer ranges (e.g. 0-10000 for talks, 10000-20000 for pages, etc.), but that gets somewhat hairy. This has the advantage of being simple.
Downside is that the consumer can't determine the talk's integer ID from the XML. There doesn't seem to be an appropriate attribute to share it in. Probably the best option will be to continue to parse the XML.