fix: Add corresponding author indication to DOCX export #272
Conversation
- Add asterisk (*) markers next to corresponding authors in author list - Add 'Correspondence:' section after affiliations with contact info - Support both plain text email and base64-encoded email64 fields - Handle multiple corresponding authors with semicolon separation Fixes issue where DOCX export was not indicating corresponding authors even though the metadata contained the information.
Code Review for PR #272: Add corresponding author indication to DOCX exportThank you for this PR! I've reviewed the changes according to the project's coding standards and best practices outlined in CLAUDE.md. Overall, this is a solid implementation that properly addresses the feature gap. Here's my detailed feedback: ✅ Strengths1. Code Quality
2. Security & Error Handling
3. Consistency
🔍 Observations & Minor Suggestions1. Exception Handling (Line 281)Current code: except Exception:
email = ""Consideration: While catching generic except (ValueError, binascii.Error, UnicodeDecodeError):
email = ""This would catch the expected exceptions from base64 decoding while allowing truly unexpected errors to surface in logs. However, this is a minor point and the current implementation is acceptable. 2. Font Size Application (Line 290)Current code: corr_para.paragraph_format.space_after = Pt(12)
corr_para.runs[-1].font.size = Pt(10)Observation: The font size is only applied to the last run. If there are multiple corresponding authors, only the last author's email will have the 10pt font size applied. Suggested improvement: corr_para.paragraph_format.space_after = Pt(12)
# Apply consistent font size to all runs in the correspondence paragraph
for run in corr_para.runs:
run.font.size = Pt(10)This ensures consistent formatting across all correspondence information. 3. Test CoverageGood: The PR description mentions manual testing and that existing tests pass. Suggestion: Consider adding a unit test specifically for the corresponding author functionality to prevent regressions: def test_corresponding_author_markers_in_docx(tmp_path):
"""Test that corresponding authors get asterisk markers and correspondence section."""
test_dir = tmp_path / "corr_author_test"
test_dir.mkdir()
main_file = test_dir / "01_MAIN.md"
main_file.write_text("""---
title: "Test Manuscript"
authors:
- name: "Jane Smith"
affiliations: ["Uni A"]
corresponding_author: true
email: "jane@example.com"
- name: "John Doe"
affiliations: ["Uni B"]
affiliations:
- shortname: "Uni A"
full_name: "University A"
- shortname: "Uni B"
full_name: "University B"
---
# Test
Content here.
""")
bib_file = test_dir / "03_REFERENCES.bib"
bib_file.write_text("@article{test2023,author={Test},title={Test},year={2023}}")
exporter = DocxExporter(manuscript_path=str(test_dir))
result_path = exporter.export()
doc = Document(str(result_path))
all_text = "\n".join([p.text for p in doc.paragraphs])
# Check for correspondence section
assert "* Correspondence:" in all_text
assert "Jane Smith" in all_text
assert "jane@example.com" in all_text
# Check for asterisk marker in author line
# (This requires checking runs with superscript formatting)
has_superscript_asterisk = False
for para in doc.paragraphs:
if "Jane Smith" in para.text:
for run in para.runs:
if run.text == "*" and run.font.superscript:
has_superscript_asterisk = True
break
assert has_superscript_asteriskThis would belong in 4. Base64 Decoding ConsistencyThe project has a dedicated from ..utils.email_encoder import decode_email
# Later in the code:
if not email:
email64 = author.get("email64", "")
if email64:
try:
email = decode_email(email64)
except ValueError:
email = ""This would provide:
🚀 Performance Considerations
📋 Testing ChecklistAccording to CLAUDE.md testing requirements:
📝 Documentation
🎯 Final RecommendationStatus: APPROVE with minor suggestions The implementation is solid and ready to merge. The suggestions above are optional improvements that could be addressed either before merge or in a follow-up PR:
Great work on bringing DOCX export to feature parity with PDF export! The code is clean, secure, and follows the project's conventions. Review conducted according to rxiv-maker CLAUDE.md guidelines, checking: code quality, security, performance, test coverage, and consistency with existing patterns. |
There was a problem hiding this comment.
Pull request overview
This PR adds corresponding author indication to DOCX exports, bringing it in line with PDF/LaTeX generation which already includes this functionality. The implementation adds asterisk markers next to corresponding authors in the author list and includes a "Correspondence:" section with contact information after the affiliations.
Key changes:
- Adds asterisk (*) superscript markers next to corresponding authors in the author list
- Adds a "Correspondence:" section after affiliations with author names and email addresses
- Supports both plain text
emailand base64-encodedemail64fields for contact information
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| corr_para.add_run(name) | ||
|
|
||
| corr_para.paragraph_format.space_after = Pt(12) | ||
| corr_para.runs[-1].font.size = Pt(10) |
There was a problem hiding this comment.
The font size is being set only on the last run of the paragraph. This means the font size (10pt) will only apply to the last author's name/email text, while the asterisk, "Correspondence:" label, and other author entries will remain at the default size. For consistent styling, the font size should be set on all runs in the correspondence paragraph, similar to how it's done for affiliations (line 255) where each run's font size is set individually.
| corr_para.runs[-1].font.size = Pt(10) | |
| for run in corr_para.runs: | |
| run.font.size = Pt(10) |
| try: | ||
| email = base64.b64decode(email64).decode("utf-8") | ||
| except Exception: | ||
| email = "" |
There was a problem hiding this comment.
The bare except clause catches all exceptions, including system exceptions like KeyboardInterrupt and SystemExit. This should be more specific by catching only the expected exception types (e.g., binascii.Error, ValueError) that could occur during base64 decoding. Using a bare except is an anti-pattern that can hide bugs and make debugging more difficult.
Code Review: PR #272 - Corresponding Author IndicationSummaryThis PR adds corresponding author indication to DOCX exports, bringing it in line with PDF/LaTeX generation. The implementation properly handles the asterisk markers and correspondence section with email decoding. ✅ Strengths1. Feature Completeness
2. Code Quality
3. User Experience
4. Testing
🔍 Observations & Suggestions1. Merge Conflict with PR #273
Recommendation:
2. Test Coverage Enhancement def test_corresponding_author_indication(sample_manuscript_path, tmp_path):
"""Test that corresponding authors are marked with asterisks."""
# Ensure test fixture has corresponding_author metadata
exporter = DocxExporter(manuscript_path=sample_manuscript_path)
result_path = exporter.export()
doc = Document(str(result_path))
all_text = "\n".join([p.text for p in doc.paragraphs])
# Check for asterisk markers in author line
author_para_text = doc.paragraphs[1].text # Adjust index as needed
assert "*" in author_para_text, "Expected asterisk marker for corresponding author"
# Check for correspondence section
assert "Correspondence:" in all_text
assert "(" in all_text and ")" in all_text # Check for email format3. Edge Case: No Email Available if email:
corr_para.add_run(f"{name} ({email})")
else:
corr_para.add_run(name)
logger.warning(f"Corresponding author {name} has no email address")4. Base64 Decoding Error Handling try:
email = base64.b64decode(email64).decode("utf-8")
except (ValueError, UnicodeDecodeError, Exception) as e:
logger.warning(f"Failed to decode email64 for {name}: {e}")
email = ""5. Font Size Inconsistency corr_para.runs[-1].font.size = Pt(10)This should probably apply to all runs in the correspondence paragraph for consistency. Consider: # Set font size for entire correspondence section
for run in corr_para.runs:
run.font.size = Pt(10)6. Documentation
🛡️ Security Considerations✅ Good security practices
Minor suggestion: The bare 🚀 Performance Considerations✅ No performance concerns
🐛 Potential Issues1. Index-based Font Size Assignment (Line 290) corr_para.runs[-1].font.size = Pt(10)This only sets the font size on the last run. If the intention is to make the entire correspondence section smaller, this should loop through all runs: for run in corr_para.runs:
run.font.size = Pt(10)2. Multiple Corresponding Authors Formatting if i > 0:
corr_para.add_run("; ") # Already has space - good!✅ This is already correct in the code. 📊 Code Metrics
🎯 Final RecommendationAPPROVE with merge coordination This is a well-implemented bug fix that:
Required Actions:
Suggested Follow-ups (not blocking):
📋 Checklist
Excellent work on bringing DOCX export to feature parity with PDF/LaTeX output! Review conducted following rxiv-maker CLAUDE.md guidelines |
|
Code Review: PR 272 - Add corresponding author indication to DOCX export Overview: This PR adds corresponding author markers (asterisks) and a Correspondence section to DOCX exports, bringing it in line with PDF/LaTeX generation. This is a focused, well-scoped bug fix. STRENGTHS:
ISSUES AND CONCERNS:
According to CLAUDE.md, the project aims for more than 80 percent test coverage.
Recommendation: Catch specific exceptions like (ValueError, UnicodeDecodeError, binascii.Error) to avoid hiding unexpected errors.
RECOMMENDATIONS: High Priority:
Medium Priority: TESTING CHECKLIST:
CODE STYLE COMPLIANCE (per CLAUDE.md):
OVERALL ASSESSMENT: This is a well-scoped bug fix that addresses a real gap in DOCX export functionality. The implementation is clean and follows existing patterns in the codebase. However, the PR needs:
Recommendation: Add tests and coordinate merge order with PR 273, then this will be ready to merge. Great work identifying and fixing this gap! The implementation is clean and follows the existing code patterns well. |
Code Review: Corresponding Author Indication in DOCX ExportThank you for this well-implemented fix! The PR successfully addresses the missing corresponding author indication in DOCX exports. StrengthsCode Quality & Consistency
Security & Robustness
Feature Completeness
Minor Observations & Suggestions1. Overly Broad Exception Handling (Low Priority) 2. Missing Direct Unit Tests (Medium Priority) 3. DRY Principle Opportunity (Low Priority) Security AnalysisNo security concerns identified: Base64 decoding is memory-safe, no SQL injection vectors, no XSS risk, no command injection, input validation present. PerformanceEfficient implementation: AuthorAffiliationProcessor extracts in single pass, no redundant loops, base64 decode is O(n) negligible. Overall AssessmentQuality Score: 4.5/5 This PR is production-ready. The suggestions above are optional improvements for follow-up PRs. Checklist Against CLAUDE.md
Great work on maintaining feature parity across export formats! |
Issue
The DOCX converter was not indicating corresponding authors, even though the metadata contained the field and email information.
Changes
Example Output
Before this fix, the author line would show:
After this fix, it shows:
Testing
Related
This brings DOCX export in line with PDF/LaTeX generation, which already indicates corresponding authors using the
\Lettersymbol.