Skip to content

2.5.2/fix/validation + drag&drop - #1689

Open
jochenklar wants to merge 9 commits into
2.5.2/releasefrom
2.5.2/fix/validation
Open

2.5.2/fix/validation + drag&drop#1689
jochenklar wants to merge 9 commits into
2.5.2/releasefrom
2.5.2/fix/validation

Conversation

@jochenklar

Copy link
Copy Markdown
Member

This PR aims to fix three things:

  1. Resolve Drag and drop/available/locked not working in nested catalog view #1685, by using PATCH for the drag and drop as well as locked/available (introducing dedicated actions for patchElement).

  2. Fixing an issue Claude pointed be to: Validators used by serializers are initialized at startup and are not copied for each request. Setting self.instance and self.serializer on the validator instances is therefore faulty. All requests write into the same attribute of the same instance. The solution is to process the serializer arg provided to __call__.

  3. For PATCH requests, where only (small) parts of the instance is in data, missing values from the instance which is updated should be used for validation. This is done by using the new get_values method in InstanceValidator.

@jochenklar jochenklar added this to the RDMO 2.5.2 milestone Jul 24, 2026
@jochenklar
jochenklar requested a review from MyPyDavid July 24, 2026 15:53
@jochenklar jochenklar self-assigned this Jul 24, 2026
@jochenklar
jochenklar requested a review from CalamityC July 24, 2026 15:53
@CalamityC

Copy link
Copy Markdown
Collaborator

Drag an element at top of the section is only possible if dragging it on the upper element.
Drag an element at the bottom of the section seems to change the hierarchy and sort that last element into the upper section.
See screen movie.

DragAndDrop.mp4

@MyPyDavid

Copy link
Copy Markdown
Member

The tests fail, I think there is some hard-code thing of the old serializer style in it:

=========================== short test summary info ============================
FAILED rdmo/core/tests/test_validators.py::test_instance_validator - AttributeError: 'InstanceValidator' object has no attribute 'serializer'
FAILED rdmo/core/tests/test_validators.py::test_instance_validator_serializer_instance - TypeError: 'InstanceValidator' object is not callable
FAILED rdmo/core/tests/test_validators.py::test_instance_validator_serializer - TypeError: 'InstanceValidator' object is not callable
FAILED rdmo/core/tests/test_validators.py::test_instance_validator_instance - AttributeError: 'InstanceValidator' object has no attribute 'serializer'
FAILED rdmo/core/tests/test_validators.py::test_instance_validator_validation_serializer_error - TypeError: 'InstanceValidator' object is not callable

@jochenklar

Copy link
Copy Markdown
Member Author

Ok, now it should work as expected. You can even move a question in a page from a questionset by targeting the small difference in indent where the page drop zone is but the question set drop zone isn't.

@CalamityC CalamityC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Works as expected.

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Comment thread rdmo/core/validators.py Outdated
if data.get('locked', False) and self.instance and self.instance.locked:
locked = self.get_value(data, instance, 'locked')
if locked and instance is not None and instance.locked:
if data.get('locked'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that this line (if data.get('locked'):) needs to be removed since this partial update feature, since how would it be possible that the following test did not raise the validation error ??!

def test_serializer_update_partial_error(db):
    attribute = Attribute.objects.get(uri='http://example.com/terms/domain/individual/single/text')
    attribute.locked = True
    attribute.save()

    validator = AttributeLockedValidator()
    serializer = AttributeSerializer(instance=attribute, partial=True)

    with pytest.raises(RestFrameworkValidationError):
        validator({
            'comment': 'Updated comment'
        }, serializer)

and pytest showed:

E       Failed: DID NOT RAISE ValidationError

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nice catch! I fixed it and added some more tests.

Comment thread rdmo/domain/tests/test_validator_locked.py
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage is 95.162%2.5.2/fix/validation into 2.5.2/release. No base build found for 2.5.2/release.

@MyPyDavid MyPyDavid changed the title 2.5.2/fix/validation 2.5.2/fix/validation + drag&drop Aug 3, 2026
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

@MyPyDavid MyPyDavid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks this fixes it and drops nicely!
Ive added one more commit for changing toggle-site also to PATCH.

const toggleAvailable = () => elementActions.patchElement('catalogs', { id: catalog.id, available: !catalog.available })
const toggleLocked = () => elementActions.patchElement('catalogs', { id: catalog.id, locked: !catalog.locked })

const toggleCurrentSite = () => elementActions.storeElement('catalogs', catalog, 'toggle-site')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would also fit to use the patchElement action for the toggleCurrentSite so I've added it (in one commit for Catalog, View and Task)

@jochenklar

Copy link
Copy Markdown
Member Author

Ok, I think its less complex now. I removed the action from storeElement and moved the toggle part into a sepeate toggleElementAction.

@jochenklar
jochenklar requested a review from MyPyDavid August 6, 2026 17:31
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.

4 participants