Skip to content

Fixes for background updates that break form validation for documents#3092

Merged
longhotsummer merged 8 commits intomainfrom
form-debug
Mar 19, 2026
Merged

Fixes for background updates that break form validation for documents#3092
longhotsummer merged 8 commits intomainfrom
form-debug

Conversation

@longhotsummer
Copy link
Contributor

There are a couple of changes here:

  • make form validation errors more obvious in the admin view, and in the logs, to trace weird validation errors that may talk about hard-to-find fields
  • the above errors were caused when background tasks and images changed underneath the hood between requests. Basically they were updated/deleted and the admin form got confused. So instead, now we only show those related objects as plain HTML and not management forms
  • those things happened because SourceFile was thinking that file had changed whenever it was saved, which triggered background updates. So this changes SourceFile to be opt-in to change tracking using track_changes()
  • adds debug logging

@github-actions
Copy link

Test Results

32 tests   - 354   32 ✅  - 344   0s ⏱️ -26s
 7 suites  -  58    0 💤  -  10 
 7 files    -  58    0 ❌ ±  0 

Results for commit a1393d8. ± Comparison against base commit 4273083.

This pull request removes 356 and adds 2 tests. Note that renamed tests count towards both.
peachjam.tests.test_adapters.IndigoAdapterTest ‑ test_import_taxonomy_tree
peachjam.tests.test_adapters.IndigoAdapterTest ‑ test_is_responsible_for_exclude_actors
peachjam.tests.test_adapters.IndigoAdapterTest ‑ test_is_responsible_for_exclude_doctypes
peachjam.tests.test_adapters.IndigoAdapterTest ‑ test_is_responsible_for_exclude_subtypes
peachjam.tests.test_adapters.IndigoAdapterTest ‑ test_is_responsible_for_include_actors
peachjam.tests.test_adapters.IndigoAdapterTest ‑ test_is_responsible_for_include_doctypes
peachjam.tests.test_adapters.IndigoAdapterTest ‑ test_is_responsible_for_include_subtypes
peachjam.tests.test_adapters.IndigoAdapterTest ‑ test_is_responsible_for_places
peachjam.tests.test_admin.TestDocumentAdminHtmlEdit ‑ test_admin_edit_source_html_updates_content_text_and_toc
peachjam.tests.test_admin.TestJournalArticleAdmin ‑ test_add_journal_article_with_journal_and_volume
…
africanlii.tests.test_views.AfricanliiViewsTest ‑ test_homepage
africanlii.tests.test_views.AfricanliiViewsTest ‑ test_legal_instrument_listing

@github-actions
Copy link

github-actions bot commented Mar 19, 2026

Test Results

388 tests  +2   378 ✅ +2   23s ⏱️ -3s
 66 suites +1    10 💤 ±0 
 66 files   +1     0 ❌ ±0 

Results for commit c99925d. ± Comparison against base commit c601df8.

♻️ This comment has been updated with latest results.

# there is a small race condition here if SourceFile is created in the db while we do this. In that case
# the task will fail and be re-tried.
source_file = getattr(doc, "source_file", None) or SourceFile(document=doc)
source_file.track_changes()
Copy link
Contributor

@mugiwaramunyi mugiwaramunyi Mar 19, 2026

Choose a reason for hiding this comment

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

Okay I understand it now....I think. A bad interaction between SourceFile's base class and the Lifecycle library, led to the weird behaviour.

Since the fix is the AttributeHooksMixin; Does this mean for classes that inherit it, they have to call track_changes() every time they want the hooks to fire, for all cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Classes that use AttributeHooksMixin must call track_changes() to setup change tracking, yes. So it's opt-in.

@longhotsummer longhotsummer merged commit 0d8d596 into main Mar 19, 2026
7 checks passed
@longhotsummer longhotsummer deleted the form-debug branch March 19, 2026 13:03
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