Skip to content

Conversation

@NarayanBavisetti
Copy link
Collaborator

@NarayanBavisetti NarayanBavisetti commented Oct 28, 2025

Description

removed the tracking (from two places) of sanitized HTML after it is sanitized.

Type of Change

  • Improvement (change that would cause existing functionality to not work as expected)

Summary by CodeRabbit

  • Bug Fixes
    • Streamlined HTML sanitization logging to stop emitting exception logs while still producing warning-level summaries.
    • Fixed cycle transfer guard so issue transfers are blocked when the source cycle has not yet ended; transfers proceed only if the source cycle is completed or has no end date.

Copilot AI review requested due to automatic review settings October 28, 2025 08:58
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes exception tracking for HTML sanitization removals. The change eliminates redundant logging where sanitization differences were being both logged as warnings and tracked as exceptions.

Key Changes:

  • Removed log_exception() call that was tracking sanitization removals as warnings

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

@makeplane
Copy link

makeplane bot commented Oct 28, 2025

Linked to Plane Work Item(s)

This comment was auto-generated by Plane

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Walkthrough

Two changes: removed a log_exception() call from HTML sanitization diff handling in the content validator; and inverted a date-check in the transfer-cycle API so transfers are blocked when the old cycle's end_date is in the future (i.e., not yet completed).

Changes

Cohort / File(s) Summary
Content validator — logging change
apps/api/plane/utils/content_validator.py
Removed log_exception() invocation from the HTML sanitization diff path when removals are detected; retained logger.warning() and unchanged control flow/return values.
Cycle transfer — validation logic flip
apps/api/plane/api/views/cycle.py
In TransferCycleIssueAPIEndpoint.post, flipped the end-date check from end_date < now to end_date > now, making transfers blocked when the old cycle's end_date is in the future (allowing transfers only for ended cycles or null end_date).

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant API as TransferCycleIssueAPIEndpoint
    participant Now as "Current time check"

    Client->>API: POST transfer request (old_cycle_id, ...)
    API->>Now: fetch now()
    Note right of API #DDEBF7: Validation (changed)
    API-->>Now: compare old_cycle.end_date > now?
    alt end_date in future (true)
        API->>Client: 400/blocked (transfer not allowed)
    else end_date past or null (false)
        API->>Client: proceed with transfer (allowed)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Mixed logical change (date comparison) and a small logging removal across two files.
  • Areas to scrutinize:
    • apps/api/plane/api/views/cycle.py: verify the new condition aligns with intended business rules and unit tests (edge cases: timezone-aware datetimes, null end_date).
    • apps/api/plane/utils/content_validator.py: ensure removal of log_exception() doesn't suppress necessary diagnostics and that warning-level logging is sufficient.

Poem

🐰 I hopped through diffs with twitchy nose and cheer,

Removed a shout, left a whisper near,
Dates now guard the gates — stay or go,
Sanitized blooms still softly glow,
A rabbit's nod to tidy code and clear! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description provides a brief statement that HTML tracking was removed from two places, but several critical sections of the template are missing or severely incomplete. While the Description section is present and the Type of Change is properly selected as "Improvement," the description lacks detail about what specific changes were made and their impact. More importantly, the Test Scenarios section is entirely missing—no information is provided about how the changes were tested—and the References section is absent, leaving no documented link to related issues beyond the ticket identifier in the title. This makes the description largely incomplete compared to the template requirements. Please expand the description with: (1) more detail about the specific changes made in both files and their rationale, (2) a Test Scenarios section describing what was tested to verify the changes work correctly, and (3) a References section with links to relevant issues (the [WEB-5263] ticket mentioned in the title should be linked here).
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "[WEB-5263] chore: removed the tracking of sanitized HTML" is directly related to one of the two file changes in the changeset (apps/api/plane/utils/content_validator.py), accurately describing the removal of exception logging in the HTML sanitization path. However, the changeset also includes modifications to apps/api/plane/api/views/cycle.py involving cycle transfer validation logic, which is not captured by the title. The title therefore only covers part of the overall changeset and is partially related rather than fully capturing the main changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore-html-sanitization

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c180a1b and a886d0b.

📒 Files selected for processing (1)
  • apps/api/plane/api/views/cycle.py (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-17T10:46:13.825Z
Learnt from: NarayanBavisetti
PR: makeplane/plane#7969
File: apps/api/plane/api/views/cycle.py:1218-1228
Timestamp: 2025-10-17T10:46:13.825Z
Learning: In the cycle transfer functionality in apps/api/plane/api/views/cycle.py, transfers should only be allowed from completed cycles (end_date < timezone.now()), not from active or draft cycles. The guard should block when end_date is None or end_date >= timezone.now().

Applied to files:

  • apps/api/plane/api/views/cycle.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Analyze (javascript)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@NarayanBavisetti NarayanBavisetti changed the title [WEB-5263] chore: removed the html sanitized tracking [WEB-5263] chore: removed the tracking of sanitized HTML Oct 28, 2025
@pushya22 pushya22 merged commit e09d986 into preview Oct 28, 2025
6 of 7 checks passed
@pushya22 pushya22 deleted the chore-html-sanitization branch October 28, 2025 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants