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

feat(interview): Value signals [6] #1190

Draft
wants to merge 1 commit into
base: interview-import-dataset
Choose a base branch
from

Conversation

jochenklar
Copy link
Member

This PR is a small spin-of from #1188, because I would like to have some feedback on the idea.

The PR adds 3 custom django signals value_created, value_deleted, and value_updated, which are fired when Values are created, updated or deleted through the interface. The front-end provides the API with the page the user in on, and this can be used in a handler to get all attributes on the current page like this:

@receiver(value_created, sender=Value)
def value_created_handler(sender, request, instance, **kwargs):
    page = Page.objects.filter(id=request.GET.get('page')).first()
    print('value_created', instance, page)


@receiver(value_updated, sender=Value)
def value_updated_handler(sender, request, instance, **kwargs):
    page = Page.objects.filter(id=request.GET.get('page')).first()
    print('value_updated', instance, page)


@receiver(value_deleted, sender=Value)
def value_deleted_handler(sender, request, instance, **kwargs):
    page = Page.objects.filter(id=request.GET.get('page')).first()
    print('value_deleted', instance, page)

The main benefit is that plugins/custom django apps could use them to manipulate answers on the same page without complicated queries.

@jochenklar jochenklar changed the base branch from main to interview-import-dataset November 12, 2024 09:56
@MyPyDavid
Copy link
Member

could it give race conditions when I click a lot of checkboxes or another user is simultaneously clicking the same checkboxes?
For your example I guess not right, since it only gets the page ??

@jochenklar
Copy link
Member Author

The signal is fired when the value is actually saved, so no additional race conditions should emerge (I think). The value itself is in instance in my example. If a value is changed by a handler and somebody else wants to edit the same value, the same ValidationError would happen as if two people would change values at the same time.

@jochenklar jochenklar changed the title Value signals feature(interview): Value signals [6] Nov 19, 2024
@jochenklar jochenklar changed the title feature(interview): Value signals [6] feat(interview): Value signals [6] Nov 19, 2024
@jochenklar jochenklar self-assigned this Nov 19, 2024
@jochenklar jochenklar added this to the RDMO 2.3.0 milestone Nov 19, 2024
@m6121
Copy link
Member

m6121 commented Nov 20, 2024

Looks good to me. Interview seems to work. Did not test it with custom signals, yet.

@jochenklar
Copy link
Member Author

An idea by @MarcoReidelbach: the front-end can also provide set_prefix, set_index, collection_index, and set_collection.

@jochenklar
Copy link
Member Author

Maybe we also add those explicetly to the signal so that they don't need to be collected from the request (which should stay nevertheless, I guess).

@cached_property
def attributes(self):
attributes = [self.attribute] if self.attribute else []
attributes += [descendant.attribute for descendant in self.descendants if descendant.attribute]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not more readable just like this? standard notation for list comprehension..

attributes += [i.attribute for i in self.descendants if i.attribute]

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.

3 participants