Skip to content

[SEC-2660] Add blurb about block visibility config to readme - #50

Merged
adam-vessey merged 1 commit into
mainfrom
document/block-visibility-condition
Jun 30, 2026
Merged

adam-vessey merged 1 commit into
mainfrom
document/block-visibility-condition

Conversation

@JojoVes

@JojoVes JojoVes commented Jun 29, 2026 •

Copy link
Copy Markdown
Contributor

Addressing need for documentation around block visibility condition sparked by a thread from the islandora slack.

Summary by CodeRabbit

  • Documentation
    • Added guidance for configuring the Embargo Notification block, including where it can be placed and when it appears based on embargo status.
    • Documented “Node is Embargoed” visibility options, including variants that ignore expired embargoes and bypassed embargoes.

@JojoVes JojoVes added the documentation Improvements or additions to documentation label Jun 29, 2026
@coderabbitai

coderabbitai Bot commented Jun 29, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The README gains a new subsection documenting the Embargo Notifications Block: its placement, visibility rules tied to an associated embargo entity, and the "Node is Embargoed" condition variants for ignoring expired or bypassed embargoes.

Changes

Embargo Notifications Block Documentation

Layer / File(s) Summary
Embargo Notifications Block config docs
README.md
Adds subsection describing block placement, embargo entity visibility rules, and "Node is Embargoed" condition options including variants that ignore expired and bypassed embargoes.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

A rabbit hops through docs so clear,
The embargo block now documented here!
Expired? Bypassed? The options show,
Which blocks appear and when to go.
🐇✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the README documentation update about block visibility configuration.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch document/block-visibility-condition

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
README.md (1)

67-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Clarify the condition selection guidance.

The current phrasing makes it difficult to determine which option corresponds to which preference for bypassed users. Consider explicitly mapping each condition to its behavior:

-If you would like the Embargo Notifications Block to stop displaying when a timed
-Embargo expires, you would want to set the "Node is Embargoed" block visibility
-condition to `Current embargoes on node (ignore expired)` or
-`Active embargoes on node (ignore bypassed)`, depending on whether you want the block
-displayed for users who have access to bypass the given Embargo.
+To hide the Embargo Notifications Block when a timed embargo has expired, use:
+• `Current embargoes on node (ignore expired)` — hides the block when expired, but
+  still shows it to users who can bypass the embargo.
+• `Active embargoes on node (ignore bypassed)` — hides the block when expired OR
+  for users who can bypass the embargo.

Also consider noting that the block's own logic already suppresses output when no embargoes apply, so this condition is primarily useful to control whether the block appears for bypassed or expired embargoes rather than letting the block render an empty container.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` around lines 67 - 72, Clarify the README guidance for the “Node is
Embargoed” visibility condition by explicitly mapping each option to its
behavior for bypassed users: use “Current embargoes on node (ignore expired)”
when the block should still show for users who can bypass the embargo, and
“Active embargoes on node (ignore bypassed)” when it should not. Also note that
the block logic already hides output when there are no applicable embargoes, so
this setting is mainly about controlling visibility for bypassed or expired
embargo cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@README.md`:
- Around line 67-72: Clarify the README guidance for the “Node is Embargoed”
visibility condition by explicitly mapping each option to its behavior for
bypassed users: use “Current embargoes on node (ignore expired)” when the block
should still show for users who can bypass the embargo, and “Active embargoes on
node (ignore bypassed)” when it should not. Also note that the block logic
already hides output when there are no applicable embargoes, so this setting is
mainly about controlling visibility for bypassed or expired embargo cases.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7cd327e0-c145-45a3-802e-7e693f300659

📥 Commits

Reviewing files that changed from the base of the PR and between 48e1dd3 and 1f59507.

📒 Files selected for processing (1)
  • README.md

@adam-vessey adam-vessey added the patch Backwards compatible bug fixes. label Jun 30, 2026
@adam-vessey

Copy link
Copy Markdown
Contributor

Just poking the README.md here, unrelated to the PHPUnit failures; though, took a swing at the failures over in #51

@adam-vessey
adam-vessey merged commit ac2951c into main Jun 30, 2026
3 of 6 checks passed
@adam-vessey
adam-vessey deleted the document/block-visibility-condition branch June 30, 2026 19:02
@github-actions

Copy link
Copy Markdown

Tag generated by PR: v1.7.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation patch Backwards compatible bug fixes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants