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

Use field courseid for logging events #203

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

melanietreitinger
Copy link
Contributor

Fixes #187

@melanietreitinger melanietreitinger force-pushed the issue-187-courseidforevents branch from 31120ff to 371895d Compare March 27, 2024 15:47
@NinaHerrmann
Copy link
Contributor

Hey Melanie thank you for your contribution.
Could you check why the tests are failing? :)
Could you elaborate on why you think we should change the context? I do not have a very strong opinion on it but from my perspective, the process is still in the system context 🤔

@melanietreitinger melanietreitinger force-pushed the issue-187-courseidforevents branch 6 times, most recently from 927397a to e3b36ad Compare April 4, 2024 07:44
@melanietreitinger
Copy link
Contributor Author

Hi Nina,

changing the context from 'system' to 'course' was necessary when using the log table field courseid - otherwise the tests failed with 'Inconsistent courseid - context combination detected'.

I fixed the tests - sorry for needing so many attempts... 🙈

@justusdieckmann
Copy link
Contributor

Hey Melanie,

thank you very much! Nothing to feel sorry about :) I think maybe it would be nicer to not store the context in the process object, but instead create it in the event_from_process function just when needed. If I remember correctly, context objects are cached pretty heavily, so even instancing a context_course object once for every step in a workflow shouldn't cause much overhead

@melanietreitinger
Copy link
Contributor Author

Hi @justusdieckmann,

I tried that in the first place, but it didn't work because when a course is deleted, the context is deleted, too, and it can't be re-created because the course is gone... 😢

@melanietreitinger melanietreitinger force-pushed the issue-187-courseidforevents branch from e3b36ad to 4625d80 Compare July 3, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvement: Better events
3 participants