Skip to content

Fix Todo.duration and Event.duration to return DURATION property when set (fixes #867)#880

Merged
niccokunzmann merged 13 commits into
collective:mainfrom
SashankBhamidi:fix-todo-duration-without-dtstart-issue-867
Jul 19, 2025
Merged

Fix Todo.duration and Event.duration to return DURATION property when set (fixes #867)#880
niccokunzmann merged 13 commits into
collective:mainfrom
SashankBhamidi:fix-todo-duration-without-dtstart-issue-867

Conversation

@SashankBhamidi

@SashankBhamidi SashankBhamidi commented Jul 16, 2025 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

Summary

This PR fixes issue #867 where Todo.duration raises an IncompleteComponent error when the DURATION property is set but DTSTART is missing. The fix ensures that both Todo.duration and Event.duration return the explicit DURATION property value when it exists, before falling back to calculated duration from start/end times.

Changes Made

  • Fixed Todo.duration (src/icalendar/cal/todo.py:215-226): Now checks for explicit DURATION property first, then falls back to calculated duration
  • Fixed Event.duration (src/icalendar/cal/event.py:305-316): Applied same fix for consistency across components
  • Added comprehensive tests (src/icalendar/tests/test_issue_867_todo_duration_fix.py): 12 test cases covering all scenarios including the exact reproduction from issue [BUG] todo.duration raises error if dtstart is missing #867
  • Updated existing tests (src/icalendar/tests/test_issue_662_component_properties.py:393-423): Separated components with DURATION property from incomplete components and added new test validating the fix
  • Updated documentation (CHANGES.rst): Added entry documenting the bug fix

Problem Solved

Before this fix, Todo.duration would always try to calculate duration from DTSTART and DUE/DTEND, causing an IncompleteComponent error when DTSTART was missing - even if an explicit DURATION property was set. This violated RFC 5545 which allows DURATION to exist without DTSTART for todos.

The caldav project was affected by this bug, as their tests expect Todo.duration to work with explicit DURATION properties.

RFC 5545 Compliance

This fix ensures proper RFC 5545 compliance:

  • For todos: DURATION can exist without DTSTART (as per RFC 5545)
  • For events: DURATION property takes precedence over calculated duration
  • Maintains 100% backward compatibility for existing functionality

Test Coverage

  • ✅ Exact reproduction of issue [BUG] todo.duration raises error if dtstart is missing #867 scenario
  • ✅ Backward compatibility with existing DTSTART+DUE/DTEND usage
  • ✅ Property precedence (explicit DURATION vs calculated)
  • ✅ Complex duration formats (ISO 8601)
  • ✅ Edge cases and error conditions
  • ✅ Both Todo and Event components covered

All existing tests pass, confirming no regressions.

Fixes #867

📚 Documentation preview 📚: https://icalendar--880.org.readthedocs.build/

@SashankBhamidi
Fix Todo.duration to return DURATION property when set
20cfc03 
Make Todo.duration return the DURATION property directly when set, rather than always trying to calculate from start and end. This allows todos with DURATION but no DTSTART to work correctly per RFC 5545.

Fixes issue #867 where Todo.duration raised IncompleteComponent error when DTSTART was missing but DURATION property was set.
@SashankBhamidi
Fix Event.duration to return DURATION property when set
7096485 
Apply same fix to Event.duration for consistency. When DURATION property is explicitly set, return it directly rather than calculating from start/end.

Addresses issue #867 comment that the same issue affects Events.
@SashankBhamidi
Add comprehensive tests for Todo and Event duration fixes
264ea86 
Test all scenarios including:
- Todo with DURATION but no DTSTART (original issue #867)
- Event with DURATION property precedence
- Backward compatibility with calculated duration
- Edge cases and complex duration formats
- Exact reproduction of caldav project failure
@SashankBhamidi
Update existing tests to reflect correct duration behavior
4c371cf 
Modify test_incomplete_event to separate components with DURATION property from those without. Components with DURATION should return the property value, not raise IncompleteComponent error.

This corrects the test expectation to match the fixed behavior.
@SashankBhamidi
Update CHANGES.rst for issue #867 fix
aee3246 
Document the bug fix for Todo.duration and Event.duration in the Bug fixes section.
@SashankBhamidi
Apply ruff formatting to issue #867 fix
2cc6d82
@coveralls

coveralls commented Jul 16, 2025 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 16350280384

Details

  • 92 of 92 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 97.441%
Totals Coverage Status
Change from base Build 16342685370: 0.03%
Covered Lines: 6994
Relevant Lines: 7167
💛 - Coveralls

@SashankBhamidi SashankBhamidi mentioned this pull request Jul 17, 2025
Merged
@SashankBhamidi
Apply code formatting with tox -e ruff as requested in review feedback
ef7f85e 
- Consistent with review pattern from other PRs
- No functional changes, only formatting improvements
@SashankBhamidi
Add backticks to technical terms in docstrings
b890f12
@stevepiercy

stevepiercy commented Jul 17, 2025

Copy link
Copy Markdown
Member

Hi @SashankBhamidi, I'm so sorry, evidently the tox -e ruff command applied reformatting on all files, but we should have done only the files you touched in the original PR. My mistake.

We should do reformatting of the entire codebase in a separate PR to keep your changes clean. Would you please revert the changes in the other files that have nothing to do with your original PR?

Again, my apologies for the bad instructions. We definitely need to document this procedure.

@SashankBhamidi
Revert "Apply code formatting with tox -e ruff as requested in review…
338e5e5 
… feedback"

This reverts commit ef7f85e.
@SashankBhamidi
Revert "Apply ruff formatting to issue #867 fix"
b1567f8 
This reverts commit 2cc6d82.
@SashankBhamidi
Apply ruff formatting to feature-specific files only
1fce2e7 
Format only the files that were modified for issue #867:
- src/icalendar/cal/event.py
- src/icalendar/cal/todo.py
- src/icalendar/tests/test_issue_867_todo_duration_fix.py
- src/icalendar/tests/test_issue_662_component_properties.py

This addresses stevepiercy's feedback to format only touched files.
@SashankBhamidi

SashankBhamidi commented Jul 17, 2025

Copy link
Copy Markdown
Member Author

Hi @SashankBhamidi, I'm so sorry, evidently the tox -e ruff command applied reformatting on all files, but we should have done only the files you touched in the original PR. My mistake.

We should do reformatting of the entire codebase in a separate PR to keep your changes clean. Would you please revert the changes in the other files that have nothing to do with your original PR?

Again, my apologies for the bad instructions. We definitely need to document this procedure.

Thanks for the clarification! I've reverted the excessive ruff formatting and now only apply formatting to the files specifically modified for this feature.

@niccokunzmann

niccokunzmann commented Jul 17, 2025

Copy link
Copy Markdown
Member

Thanks for the clarification! I've reverted the excessive ruff formatting and now only apply formatting to the files specifically modified for this feature.

If you install the pre-commit, then this would be done on every commit automatically.

niccokunzmann
niccokunzmann approved these changes Jul 17, 2025
View reviewed changes

@niccokunzmann niccokunzmann left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@SashankBhamidi Thanks for all your contributions! Would you like to resolve the merge conflict?

This looks good from my side, too and should fix the issue.

Comment thread src/icalendar/tests/test_issue_867_todo_duration_fix.py Outdated
@SashankBhamidi
Use pytest.mark.parametrize for Event/Todo duration tests
7cd3915 
Replace duplicate test methods with parametrized tests to reduce code duplication as suggested by niccokunzmann in review feedback.
@SashankBhamidi
Resolve merge conflicts with upstream/main
72cd9a5 
- Merge bug fix entries in CHANGES.rst for issues #844 and #867
- Merge docstring improvements in Event.duration property
@SashankBhamidi

SashankBhamidi commented Jul 17, 2025

Copy link
Copy Markdown
Member Author

@niccokunzmann Done! I've implemented the parametrized tests and resolved the merge conflicts. The duplicate Event/Todo test functions are now combined using @pytest.mark.parametrize("component_class", [Event, Todo]) as suggested.

@niccokunzmann niccokunzmann merged commit 67199d7 into collective:main Jul 19, 2025
18 checks passed
@niccokunzmann

niccokunzmann commented Jul 19, 2025

Copy link
Copy Markdown
Member

Thank you very much!

@SashankBhamidi SashankBhamidi deleted the fix-todo-duration-without-dtstart-issue-867 branch August 19, 2025 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@niccokunzmann niccokunzmann Awaiting requested review from niccokunzmann niccokunzmann is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] todo.duration raises error if dtstart is missing

4 participants

@SashankBhamidi @coveralls @stevepiercy @niccokunzmann