Refactor: Unify book provider download options template #11615

Ritesh-251 · 2025-12-20T17:23:59Z

🧩 Summary

This PR unifies the rendering of book provider download options by replacing multiple provider-specific templates with a single shared template that renders directly from get_acquisitions().

The goal is to remove duplicated template logic and centralize download option definitions within each provider’s acquisition data.

Fixes #11274

✨ What’s Changed

Unified template
- Added book_providers/download_options.html, which iterates over a list of Acquisition objects and renders download links generically.
Acquisition model update
- Added an optional label field to the Acquisition dataclass.
- Updated from_json and from_opds_json to populate this field, preserving existing UI text cleanly.
Provider updates
- Updated the following providers to return complete acquisition lists (with labels) matching previous behavior:
  - CitaPress
  - OpenStax
  - Project Gutenberg
  - Project Runeberg
  - Standard Ebooks
  - LibriVox
  - Wikisource
  - Internet Archive
Rendering refactor
- Updated AbstractBookProvider.render_download_options to use the unified template.
- Preserved Internet Archive behavior by filtering out the “Read Online” link (handled by the main CTA).
- Disabled render_download_options for BetterWorldBooksProvider to avoid purchase links appearing in the download options section.
Cleanup
- Removed obsolete provider-specific download option templates now replaced by the unified implementation.

🔍 Behavior Notes

No functional changes to download availability or user experience are intended.
Existing provider-specific behaviors have been preserved where applicable (e.g., Internet Archive “Read Online” handling).

🧪 Testing

Manually verified download option rendering for:
- Internet Archive
- LibriVox
- OpenStax
- Project Gutenberg
- Standard Ebooks
No automated test changes were required.

🙏 Reviewer Notes

Happy to adjust if there’s a preference for handling labels, filtering, or provider-specific affordances differently.

for more information, see https://pre-commit.ci

Copilot

Pull request overview

This PR refactors the book provider download options rendering by consolidating multiple provider-specific templates into a single unified template that works with the Acquisition dataclass. The refactor aims to reduce code duplication and centralize download option definitions within each provider's get_acquisitions() method.

Key Changes:

Added a unified book_providers/download_options.html template that renders acquisitions generically
Extended the Acquisition dataclass with an optional label field to preserve UI text
Updated 8 book providers (CitaPress, OpenStax, Project Gutenberg, Project Runeberg, Standard Ebooks, LibriVox, Wikisource, Internet Archive) to return complete acquisition lists with labels

Reviewed changes

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

Show a summary per file

File	Description
`openlibrary/book_providers.py`	Added `label` field to Acquisition dataclass; updated `from_json` and `from_opds_json` to populate labels; refactored `render_download_options` in AbstractBookProvider and InternetArchiveProvider; updated all 8 providers' `get_acquisitions()` methods to return complete acquisition lists with labels and proper URLs
`openlibrary/templates/book_providers/download_options.html`	New unified template that iterates over acquisitions and renders download links with fallback label logic
`openlibrary/templates/book_providers/wikisource_download_options.html`	Deleted provider-specific template (replaced by unified template)
`openlibrary/templates/book_providers/standard_ebooks_download_options.html`	Deleted provider-specific template (replaced by unified template)
`openlibrary/templates/book_providers/runeberg_download_options.html`	Deleted provider-specific template (replaced by unified template)
`openlibrary/templates/book_providers/openstax_download_options.html`	Deleted provider-specific template (replaced by unified template)
`openlibrary/templates/book_providers/librivox_download_options.html`	Deleted provider-specific template (replaced by unified template)
`openlibrary/templates/book_providers/ia_download_options.html`	Deleted provider-specific template (replaced by unified template)
`openlibrary/templates/book_providers/gutenberg_download_options.html`	Deleted provider-specific template (replaced by unified template)
`openlibrary/templates/book_providers/cita_press_download_options.html`	Deleted provider-specific template (replaced by unified template)

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

Copilot · 2025-12-20T17:28:12Z

openlibrary/book_providers.py

        return [
            Acquisition(
                access='open-access',
                format='web',
                price=None,
-                url=f'https://runeberg.org/{self.get_best_identifier(ed_or_solr)}/',
+                url=f'https://runeberg.org/{runeberg_id}.zip',
                provider_name=self.short_name,
-            )
+                label='Scanned images',
+            ),
+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=f'https://runeberg.org/download.pl?mode=jpgzip&work={runeberg_id}',
+                provider_name=self.short_name,
+                label='Color images',
+            ),
+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=f'https://runeberg.org/download.pl?mode=html&work={runeberg_id}',
+                provider_name=self.short_name,
+                label='HTML',
+            ),
+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=f'https://runeberg.org/download.pl?mode=txtzip&work={runeberg_id}',
+                provider_name=self.short_name,
+                label='Text and index files',
+            ),
+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=f'https://runeberg.org/download.pl?mode=ocrtext&work={runeberg_id}',
+                provider_name=self.short_name,
+                label='OCR text',
+            ),
+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=f'https://runeberg.org/download.pl?mode=work&work={runeberg_id}',
+                provider_name=self.short_name,
+                label='More at Project Runeberg',
+            ),
        ]


All acquisitions for Project Runeberg use format='web', even though they represent distinct file types (Scanned images zip, Color images, HTML, Text and index files, OCR text). This makes the format field less useful for tracking and categorization. Consider using more specific format values or adding the format literal types to support these file types.

Copilot · 2025-12-20T17:28:13Z

openlibrary/book_providers.py

+        acqs.append(
            Acquisition(
                access='open-access',
-                format='audio',
+                format='web',
                price=None,
-                url=f'https://librivox.org/{self.get_best_identifier(ed_or_solr)}',
+                url=f'https://librivox.org/rss/{librivox_id}',
                provider_name=self.short_name,
+                label='RSS Feed',
            )
-        ]
+        )
+        acqs.append(
+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=f'https://librivox.org/{librivox_id}',
+                provider_name=self.short_name,
+                label='More at LibriVox',
+            )
+        )
+        return acqs


The format field for RSS Feed and "More at LibriVox" acquisitions is set to 'web'. While 'web' is appropriate for the "More at" link, an RSS feed is not a web reading interface but a syndication format. This reduces semantic clarity.

Copilot · 2025-12-20T17:28:13Z

openlibrary/book_providers.py

+            ),
            Acquisition(
                access='open-access',
                format='web',


The format field for MOBI (for Kindle) acquisition is set to 'web', but MOBI is a distinct ebook format. Using 'web' for MOBI reduces semantic clarity and makes analytics tracking less precise. This is the same issue as in the Internet Archive provider.

Suggested change

format='web',

format='mobi',

Copilot · 2025-12-20T17:28:13Z

openlibrary/templates/book_providers/download_options.html

+<p class="cta-section-title">$_("Download Options")</p>
+<ul class="ebook-download-options">
+    $for acq in acquisitions:
+        <li><a href="$acq.url" data-ol-link-track="Download|$(acq.format)_$(acq.provider_name)" title="$_('Download from') $acq.provider_name">$(get_label(acq))</a></li>


The title attribute construction will result in incorrect output when labels contain special formatting. For labels like "More at LibriVox" or "Read at Wikisource", the title will be "Download from librivox" or "Download from wikisource", which is semantically incorrect - these are not downloads but rather links to read at external sites. The title should be more contextually appropriate or conditionally set based on whether it's actually a download link.

Suggested change

<li><a href="$acq.url" data-ol-link-track="Download|$(acq.format)_$(acq.provider_name)" title="$_('Download from') $acq.provider_name">$(get_label(acq))</a></li>

<li><a href="$acq.url" data-ol-link-track="Download|$(acq.format)_$(acq.provider_name)" title="$if acq.format == 'web':$_('Read at')$else:$_('Download from')$endif $acq.provider_name">$(get_label(acq))</a></li>

Copilot · 2025-12-20T17:28:13Z

openlibrary/templates/book_providers/download_options.html

+<p class="cta-section-title">$_("Download Options")</p>
+<ul class="ebook-download-options">
+    $for acq in acquisitions:
+        <li><a href="$acq.url" data-ol-link-track="Download|$(acq.format)_$(acq.provider_name)" title="$_('Download from') $acq.provider_name">$(get_label(acq))</a></li>


The tracking attribute data-ol-link-track also concatenates with provider_name which can be None. When provider_name is None, this will produce invalid tracking data like "Download|pdf_None". The template should handle None values or ensure provider_name is always set.

Suggested change

<li><a href="$acq.url" data-ol-link-track="Download|$(acq.format)_$(acq.provider_name)" title="$_('Download from') $acq.provider_name">$(get_label(acq))</a></li>

<li><a href="$acq.url" data-ol-link-track="Download|$(acq.format)$('_' + acq.provider_name if acq.provider_name else '')" title="$_('Download from')$(': ' + acq.provider_name if acq.provider_name else '')">$(get_label(acq))</a></li>

Copilot · 2025-12-20T17:28:14Z

openlibrary/book_providers.py

+            if txt := edition.get_ia_download_link('_djvu.txt'):
+                acqs.append(
+                    Acquisition(
+                        access=access,
+                        format='web',
+                        price=None,
+                        url=txt,
+                        provider_name=self.short_name,
+                        label='Plain text',
+                    )
+                )


The format field is set to 'web' for plain text files, but the Acquisition dataclass only allows 'web', 'pdf', 'epub', or 'audio' as valid formats (line 80). Plain text is a distinct ebook format and should likely have its own format type. This inconsistency affects tracking and semantic clarity.

Copilot · 2025-12-20T17:28:14Z

openlibrary/book_providers.py

        return [
            Acquisition(
                access='open-access',
                format='web',
                price=None,
-                url=f'https://www.gutenberg.org/ebooks/{self.get_best_identifier(ed_or_solr)}',
+                url=url,
                provider_name=self.short_name,
-            )
+                label='HTML',
+            ),
+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=url,
+                provider_name=self.short_name,
+                label='Plain text',
+            ),
+            Acquisition(
+                access='open-access',
+                format='epub',
+                price=None,
+                url=url,
+                provider_name=self.short_name,
+                label='ePub',
+            ),
+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=url,
+                provider_name=self.short_name,
+                label='Kindle',
+            ),
+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=url,
+                provider_name=self.short_name,
+                label='More at Project Gutenberg',
+            ),
        ]


The format field for multiple acquisitions is set to 'web' when they represent specific downloadable file formats (HTML, Plain text, Kindle). The format 'web' should be reserved for web-based reading or generic landing pages. Using 'web' for distinct file formats reduces the semantic value of the format field and makes analytics tracking less precise.

Copilot · 2025-12-20T17:28:14Z

openlibrary/book_providers.py

+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=f'{base_url}/downloads/{flat_id}.azw3',
+                provider_name=self.short_name,
+                label='Kindle (azw3)',
+            ),
+            Acquisition(
+                access='open-access',
+                format='epub',
+                price=None,
+                url=f'{base_url}/downloads/{flat_id}.kepub.epub',
+                provider_name=self.short_name,
+                label='Kobo (kepub)',
+            ),
+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=f'https://github.com/standardebooks/{flat_id}',
+                provider_name=self.short_name,
+                label='Source Code',
+            ),
+            Acquisition(
+                access='open-access',
+                format='web',
+                price=None,
+                url=base_url,
+                provider_name=self.short_name,
+                label='More at Standard Ebooks',
            ),
        ]


The format field for Kindle (azw3) and Source Code acquisitions is set to 'web', but azw3 is a distinct ebook format. Using 'web' for these reduces semantic clarity and makes analytics tracking less precise. Consider adding support for these format types in the Acquisition literal or use more appropriate existing types.

Copilot · 2025-12-20T17:28:14Z

openlibrary/templates/book_providers/download_options.html

+<p class="cta-section-title">$_("Download Options")</p>
+<ul class="ebook-download-options">
+    $for acq in acquisitions:
+        <li><a href="$acq.url" data-ol-link-track="Download|$(acq.format)_$(acq.provider_name)" title="$_('Download from') $acq.provider_name">$(get_label(acq))</a></li>


The title attribute concatenates with provider_name which can be None (as defined in the Acquisition dataclass line 83). When provider_name is None, this will produce a title like "Download from None", which is not user-friendly. The template should handle None values appropriately.

Suggested change

<li><a href="$acq.url" data-ol-link-track="Download|$(acq.format)_$(acq.provider_name)" title="$_('Download from') $acq.provider_name">$(get_label(acq))</a></li>

$if acq.provider_name:

<li><a href="$acq.url" data-ol-link-track="Download|$(acq.format)_$(acq.provider_name)" title="$_('Download from') $acq.provider_name">$(get_label(acq))</a></li>

$else:

<li><a href="$acq.url" data-ol-link-track="Download|$(acq.format)_$(acq.provider_name)" title="$_('Download')">$(get_label(acq))</a></li>

Copilot · 2025-12-20T17:28:15Z

openlibrary/book_providers.py

+            acqs.append(
+                Acquisition(
+                    access=access,
+                    format='web',
+                    price=None,
+                    url=edition.url('/daisy'),
+                    provider_name=self.short_name,
+                    label='DAISY',
+                )
+            )


The format field is set to 'web' for DAISY files, but the Acquisition dataclass only allows 'web', 'pdf', 'epub', or 'audio' as valid formats (line 80). DAISY is a distinct accessibility-focused ebook format and should likely have its own format type. This inconsistency affects tracking and semantic clarity.

for more information, see https://pre-commit.ci

Ritesh-251 · 2025-12-23T08:46:38Z

@cdrini Can you review it pls

Refactor: Unify book provider download options template

a09ea59

Copilot AI review requested due to automatic review settings December 20, 2025 17:24

github-actions bot assigned cdrini Dec 20, 2025

Copilot started reviewing on behalf of Ritesh-251 December 20, 2025 17:24 View session

[pre-commit.ci] auto fixes from pre-commit.com hooks

8a92bdf

for more information, see https://pre-commit.ci

Copilot AI reviewed Dec 20, 2025

View reviewed changes

Ritesh-251 and others added 2 commits December 20, 2025 23:17

Fix: Address PR review comments

53e07d5

[pre-commit.ci] auto fixes from pre-commit.com hooks

bd76d02

for more information, see https://pre-commit.ci

github-actions bot added the Needs: Response Issues which require feedback from lead label Dec 24, 2025

	<li><a href="$acq.url" data-ol-link-track="Download\|$(acq.format)_$(acq.provider_name)" title="$_('Download from') $acq.provider_name">$(get_label(acq))</a></li>
	<li><a href="$acq.url" data-ol-link-track="Download\|$(acq.format)_$(acq.provider_name)" title="$if acq.format == 'web':$_('Read at')$else:$_('Download from')$endif $acq.provider_name">$(get_label(acq))</a></li>

Uh oh!

Refactor: Unify book provider download options template #11615

Are you sure you want to change the base?

Refactor: Unify book provider download options template #11615

Uh oh!

Conversation

Ritesh-251 commented Dec 20, 2025

🧩 Summary

✨ What’s Changed

🔍 Behavior Notes

🧪 Testing

🙏 Reviewer Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

Ritesh-251 commented Dec 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants