Skip to content

Directory: Preserve attachments on entry save with past publication date - #2633

Open
Tschuppi81 wants to merge 12 commits into
masterfrom
bugfix/3366-directories-save-entry-with-publi
Open

Directory: Preserve attachments on entry save with past publication date#2633
Tschuppi81 wants to merge 12 commits into
masterfrom
bugfix/3366-directories-save-entry-with-publi

Conversation

@Tschuppi81

@Tschuppi81 Tschuppi81 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Directory: Preserve attachments on entry save with past publication date for single and multi upload fields

Uploading a file and saving/editing a directory entry with a publication date in the past triggered a validation error that discarded the attachments. The upload now survives the re-render (new entries) and edits keep the persisted files.

TYPE: Bugfix
LINK: OGC-3366

@Tschuppi81 Tschuppi81 changed the title Bugfix/3366 directories save entry with publi Directory: Preserve attachments on entry save with past publication date Aug 14, 2026
@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.15686% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.88%. Comparing base (3a3b4ff) to head (a9f6db9).
⚠️ Report is 10 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/onegov/directory/models/directory.py 85.71% 3 Missing ⚠️
src/onegov/org/forms/fields.py 0.00% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/onegov/form/fields.py 94.48% <100.00%> (+0.38%) ⬆️
src/onegov/form/parser/form.py 96.70% <100.00%> (ø)
src/onegov/form/widgets.py 86.36% <100.00%> (+0.41%) ⬆️
src/onegov/org/views/directory.py 84.66% <ø> (-0.13%) ⬇️
src/onegov/org/forms/fields.py 57.98% <0.00%> (ø)
src/onegov/directory/models/directory.py 92.98% <85.71%> (-0.24%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a3b4ff...a9f6db9. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Tschuppi81
Tschuppi81 requested a review from Daverball August 14, 2026 13:37

@Daverball Daverball 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.

Overall it seems like a reasonable solution, but it also makes an already messy situation even messier. I'm also not happy that some of the implementation details of directories have now leaked into the generic form module.

If we can clean this up and improve the situation, I'll be a lot happier.

Comment thread src/onegov/form/fields.py Outdated
Comment on lines +383 to +394
if raw_data.startswith('@'):
# reference to a persisted file: keep the loaded metadata
# (size, mimetype) so display and validation still work
original = self.object_data
if isinstance(original, dict) and \
original.get('data') == raw_data:
self.data = cast('StrictFileDict', original)
else:
self.data = {
'data': raw_data,
'filename': str(valuelist[2]),
}

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'm not super happy that we have moved an implementation detail of how directory entries store their uploaded files into the generic UploadField. If you can make this work without adding any special-casing here, that would be a preferred solution.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, in the generic UploadField makes only sense if we face the same issue for other form too.

@Tschuppi81 Tschuppi81 Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It turns out plain form submissions rely on this the same way directories do: saved uploads are referenced as @ and resent on edit — without the @ branch they get decoded as inline data and lost. Added test_pending_submission_file_survives_edit to cover it. So UploadField is the right location and fixes Directories and Form Submissions.

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.

If that's the case it makes even more sense to define formcode specific subclasses, since we still use UploadField in other places that aren't form submissions. And we probably want to refactor things so that file handling is reusable between FormSubmissionCollection.update and Directory.update, otherwise we will always have to fix bugs in two locations and be very careful to keep things in sync, which they may already not be.

'filename': data['filename'],
'mimetype': new_file.reference.file.content_type,
'size': new_file.reference.file.content_length
}

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.

It's a little unfortunate how things here got even more messy and repetitive, I think we're at the point where a refactor might be necessary to keep things readable/understandable.

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.

2 participants