Bugfix/ogc 3003 fix migrate links by Tschuppi81 · Pull Request #2428 · OneGov/onegov-cloud

Tschuppi81 · 2026-03-27T18:12:12Z

Org: Fix migrate links tool

Use re.subn to count URL occurrences per field (not just whether a field changed), and wrap the
substituted value in Markup when the original was Markup to prevent double-escaping HTML
entities.

TYPE: Bugfix
LINK: ogc-3003

…ence count what the text suggests

linear · 2026-03-27T18:12:15Z

OGC-3003 Migration Links zerstört Inhalte

codecov · 2026-03-27T18:16:39Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.34%. Comparing base (72b3b75) to head (7e3b502).
✅ All tests successful. No failed tests found.

Additional details and impacted files

Files with missing lines	Coverage Δ
src/onegov/org/management.py	`92.26% <100.00%> (+1.12%)`	⬆️
src/onegov/org/models/page.py	`94.95% <100.00%> (+0.08%)`	⬆️
src/onegov/org/views/settings.py	`86.93% <ø> (+6.33%)`	⬆️

... and 3 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72b3b75...7e3b502. 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.

This reverts commit 3810420.

Copilot

Pull request overview

Fixes the /migrate-links link migration behavior to (1) count all URL occurrences rather than just “field changed”, and (2) preserve Markup values during substitution to avoid unwanted escaping changes.

Changes:

Update link migration to use re.subn for accurate per-occurrence counting and propagate Markup for migrated HTML fields.
Add a Town6 integration test covering the /migrate-links dry-run and actual migration behavior.
Add by_title() helpers on TopicCollection/NewsCollection to simplify retrieving fixtures in tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
tests/onegov/town6/test_views_settings.py	Adds an end-to-end test for `/migrate-links` covering detection and migration of multiple links.
src/onegov/org/models/page.py	Adds `by_title()` convenience lookups to `TopicCollection` and `NewsCollection`.
src/onegov/org/management.py	Counts replacements with `subn` and preserves `Markup` when mutating stored HTML content.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/onegov/org/models/page.py

tests/onegov/town6/test_views_settings.py

Copilot · 2026-03-27T18:39:08Z

tests/onegov/town6/test_views_settings.py

+    topic_text_replaced = topic_text.replace('foo.ch', 'localhost')
+    assert topic_text_replaced == topic_text_new
+    news_text_replaced = news_text.replace('foo.ch', 'localhost')


The expected replacement domain is hard-coded as 'localhost'. This makes the test brittle if the test server host/domain is ever changed (e.g. to 127.0.0.1 or a custom host). Prefer deriving the expected new domain from the app/request (the same value used by the migration, i.e. request.domain).

Suggested change

topic_text_replaced = topic_text.replace('foo.ch', 'localhost')

assert topic_text_replaced == topic_text_new

news_text_replaced = news_text.replace('foo.ch', 'localhost')

# derive the new domain from the migrated topic text to avoid

# hard-coding values like 'localhost' which depend on the test host

new_domain = topic_text_new.split('https://', 1)[1].split('/abc', 1)[0]

topic_text_replaced = topic_text.replace(old_domain, new_domain)

assert topic_text_replaced == topic_text_new

news_text_replaced = news_text.replace(old_domain, new_domain)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Daverball

I initially thought your fix was incorrect, but I misread parts of it. So the fix is fine, but the tests could be improved a little.

Daverball · 2026-03-27T21:22:13Z

tests/onegov/town6/test_views_settings.py

+
+    # create topic
+    topic = Topic(title='Foo Topic', name='foo-topic')
+    topic.text = '<p>Wow, https://foo.ch/abc is a great page!</p>'


Suggested change

topic.text = 'Wow, https://foo.ch/abc is a great page!'

topic.text = Markup('Wow, https://foo.ch/abc is a great page!')

Make sure to use Markup here, otherwise  will get escaped to 

Daverball · 2026-03-27T21:23:07Z

tests/onegov/town6/test_views_settings.py

+    topic = Topic(title='Foo Topic', name='foo-topic')
+    topic.text = '<p>Wow, https://foo.ch/abc is a great page!</p>'
+    session.add(topic)
+    topic_text = str(topic.text)


Suggested change

topic_text = str(topic.text)

topic_text = topic.text

assert topic_text is not None

The type checker error was due to this potentially being None, not because it wasn't a str.

Daverball · 2026-03-27T21:23:46Z

tests/onegov/town6/test_views_settings.py

+    news.text = ('<p>Big news https://foo.ch/big-news and bigger news'
+                 'can be found here https://foo.ch/bigger-news</p>')


Suggested change

news.text = ('Big news https://foo.ch/big-news and bigger news'

'can be found here https://foo.ch/bigger-news')

news.text = Markup('Big news https://foo.ch/big-news and bigger news'

'can be found here https://foo.ch/bigger-news')

Daverball · 2026-03-27T21:24:29Z

tests/onegov/town6/test_views_settings.py

+    news.text = ('<p>Big news https://foo.ch/big-news and bigger news'
+                 'can be found here https://foo.ch/bigger-news</p>')
+    session.add(news)
+    news_text = str(news.text)


Suggested change

news_text = str(news.text)

news_text = news.text

assert news_text is not None

Daverball · 2026-03-27T21:24:53Z

tests/onegov/town6/test_views_settings.py

+    def get_topic_text() -> str:
+        t = TopicCollection(session).by_title('Foo Topic')
+        assert t is not None and t.text is not None
+        return str(t.text)
+
+    def get_news_text() -> str:
+        n = NewsCollection(request).by_title('Big News')
+        assert n is not None and n.text is not None
+        return str(n.text)


Suggested change

def get_topic_text() -> str:

t = TopicCollection(session).by_title('Foo Topic')

assert t is not None and t.text is not None

return str(t.text)

def get_news_text() -> str:

n = NewsCollection(request).by_title('Big News')

assert n is not None and n.text is not None

return str(n.text)

def get_topic_text() -> Markup:

t = TopicCollection(session).by_title('Foo Topic')

assert t is not None and t.text is not None

return t.text

def get_news_text() -> Markup:

n = NewsCollection(request).by_title('Big News')

assert n is not None and n.text is not None

return n.text

Daverball · 2026-03-27T21:27:56Z

tests/onegov/town6/test_views_settings.py

+    assert old_domain not in news_text_new
+
+    assert topic_text.replace('foo.ch', 'localhost') == topic_text_new
+    assert news_text.replace('foo.ch', 'localhost') == news_text_new


I think you should use an <a> tag in at least one of your tests, to make sure those remain intact as well, it should actually be very rare to encounter a naked link, they should almost always be part of an <a> tag in these fields.

Tschuppi81 added 5 commits March 27, 2026 11:41

Adding test

8685d5f

Identified issue that count is a document count rather than an occurr…

640950e

…ence count what the text suggests

Count all replaced occurrences

846f6d2

Reproduce issue in test

83a3980

Wrap new value in Markup if the original value was Markup

997edc3

Fix linter issue

e0fed86

Tschuppi81 force-pushed the bugfix/ogc-3003-fix-migrate-links branch from 3c4f80c to e0fed86 Compare March 27, 2026 18:22

Tschuppi81 added 2 commits March 27, 2026 14:30

Ignore bandit warning as the value was already Markup before

f075f1c

Cleanup

627ebae

Tschuppi81 requested a review from Copilot March 27, 2026 18:33

Copilot started reviewing on behalf of Tschuppi81 March 27, 2026 18:34 View session

Revert "Remove migrate links from settings view"

8bed955

This reverts commit 3810420.

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Tschuppi81 and others added 4 commits March 27, 2026 14:43

Topic by title to use subset for filtering

4ada6f8

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

News by title to use subset for filtering

cead900

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

News require a root node

df1e866

Fix various linter issues

7e3b502

Tschuppi81 requested a review from Daverball March 27, 2026 19:02

Daverball requested changes Mar 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/ogc 3003 fix migrate links#2428

Bugfix/ogc 3003 fix migrate links#2428
Tschuppi81 wants to merge 13 commits intomasterfrom
bugfix/ogc-3003-fix-migrate-links

Tschuppi81 commented Mar 27, 2026 •

edited

Loading

Uh oh!

linear bot commented Mar 27, 2026

Uh oh!

codecov bot commented Mar 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Daverball left a comment •

edited

Loading

Uh oh!

Daverball Mar 27, 2026

Uh oh!

Daverball Mar 27, 2026

Uh oh!

Daverball Mar 27, 2026

Uh oh!

Daverball Mar 27, 2026

Uh oh!

Daverball Mar 27, 2026

Uh oh!

Daverball Mar 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-    topic_text_replaced = topic_text.replace('foo.ch', 'localhost')
-    assert topic_text_replaced == topic_text_new
-    news_text_replaced = news_text.replace('foo.ch', 'localhost')
+    # derive the new domain from the migrated topic text to avoid
+    # hard-coding values like 'localhost' which depend on the test host
+    new_domain = topic_text_new.split('https://', 1)[1].split('/abc', 1)[0]
+    topic_text_replaced = topic_text.replace(old_domain, new_domain)
+    assert topic_text_replaced == topic_text_new
+    news_text_replaced = news_text.replace(old_domain, new_domain)

	topic.text = '<p>Wow, https://foo.ch/abc is a great page!</p>'
	topic.text = Markup('<p>Wow, https://foo.ch/abc is a great page!</p>')

	topic_text = str(topic.text)
	topic_text = topic.text
	assert topic_text is not None

		news.text = ('<p>Big news https://foo.ch/big-news and bigger news'
		'can be found here https://foo.ch/bigger-news</p>')

	news_text = str(news.text)
	news_text = news.text
	assert news_text is not None

Conversation

Tschuppi81 commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear bot commented Mar 27, 2026

Uh oh!

codecov bot commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Daverball left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Daverball Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Daverball Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Daverball Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Daverball Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Daverball Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Daverball Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Tschuppi81 commented Mar 27, 2026 •

edited

Loading

codecov bot commented Mar 27, 2026 •

edited

Loading

Daverball left a comment •

edited

Loading