Fix/10285 correct serialization of download url property for document model with direct wagtaildocs serve method config

[Swojak-A]

Fix #10285

Fixing the serialization of download_url property in Document model in case of WAGTAILDOCS_SERVE_METHOD = "direct" config turned on. Issue: click here

Please check the following:

• Do the tests still pass?¹
• Does the code comply with the style guide?
  • Run make lint from the Wagtail root.
• For Python changes: Have you added tests to cover the new/fixed behaviour?
• For front-end changes: Did you test on all of Wagtail’s supported environments?²
  • Please list the exact browser and operating system versions you tested:
  • Please list which assistive technologies ³ you tested:
• For new features: Has the documentation been updated accordingly?

Please describe additional details for testing this change.

Footnotes

1. Development Testing ↩

2. Browser and device support ↩

3. Accessibility Target ↩

[squash-labs]

Manage this branch in Squash

Test this branch here: https://swojak-afix10285-correct-seria-sbgzv.squash.io

[gasman]

Thanks for picking this up @Swojak-A! Just a couple of suggestions...

• Rather than adding a special case for WAGTAILDOCS_SERVE_METHOD == 'direct' within DocumentDownloadUrlField, I think it would be better to fix the get_full_url helper function to not add the base URL if it's already been passed a full URL. (This can simply check for it starting with http:// or https://, like Django does here.) That way, we can be confident that this bug won't arise again in some other scenario in future (e.g. we introduce a new WAGTAILDOCS_SERVE_METHOD option that also returns absolute URLs).
• The test code might be over-complicating things a bit - since we have control over the test setup and know exactly what URL we expect to get back, we can just do a self.assertEqual(download_url, "http://remotestorage.com/media/...") rather than the "is this a valid URL with only one http" checks.

[Swojak-A]

@gasman No problem. I will modify the Pull Request accordingly.

[lb-]

@Swojak-A when you get a moment, please also update the first line of the description. The content Fix #... is there so you can replace the ... with the actual issue number.

Thanks for working through the feedback.

[Swojak-A]

@gasman I've updated the Pull Request. Let me know pls, if there's still something to change.

[gasman]

Thanks @Swojak-A! Have now merged in 0447b25, with some small tweaks:

• Changed the startswith check to look for http:// or https://, like Django does - to avoid the possibility that a local path might legitimately start with http. (I don't think there's ever a reason for local paths to not start with /, but best not to risk it...)
• Removed the now-unused _assert_valid_url method
• Changed the test assertions to test the whole URL, rather than startswith - since the whole URL is predictable, and this will catch any similar bugs in future where something is wrongly prepended to the string.

I've added you to the contributors list as Swojak-A - please let me know if you'd like this changing to a different name.