Refactor opr file stucture and code#2345
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Move parse_keys, sort_in_category and get_all_field_descriptions out of TablePeerReviewView as pure, unit-testable functions. Drops the unused 'table' arg from sort_in_category. Behavior-preserving. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Moves the reviewer/contributor POST branching out of the views 1:1 (behavior-preserving). Raises ContributorNotFoundError for the no-table-holder case. Duplicate-PeerReviewManager bug intentionally left for S3/S4. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
TablePeerReviewView/TablePeerRreviewContributorView POST handlers now parse the request and delegate to ReviewService; metadata shaping uses the extracted serializer. Removes the now-unused inline methods and imports. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Pins current behavior of merge_field_reviews, recursive_update and process_review_data (17 SimpleTestCase tests, no DB). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Pins set_next_reviewer/whos_turn, save vs update paths, the contributor==reviewer guard, load and days_open (14 TestCase tests). Documents the duplicate-manager bug. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
11 SimpleTestCase tests for parse_keys, sort_in_category and get_all_field_descriptions (previously untested). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Two OEDB-free tests covering contributor submit (merge + turn toggle) and save (SAVED, no toggle). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Adds the ReviewRound model (append-only per-turn log; opr FK, sequence, role, actor, action, field_reviews, sets_finished) that PeerReview.review will be projected from. Also fixes PeerReview.save() to use get_or_create for the PeerReviewManager so a review keeps exactly one manager (was creating a new one on every call). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Pure functions: project_review/build_reviews_from_rounds rebuild the review JSON from rounds with fieldReview always a list; compute_round_delta keeps only new contributions vs prior rounds; reconstruct_rounds_from_review rebuilds rounds from a legacy review blob (for backfill). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
submit/finish now compute the per-turn delta, append a ReviewRound, and rebuild PeerReview.review via project_review. Drops merge_field_reviews and the payload-extend hack. _apply_finished merges the projected values into the live metadata + snapshot. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Reviewer-only: all fields accepted -> offer Finish; any suggestion/deny -> keep Submit enabled (ping-pong). Stops disabling submit and showing the badge UI prematurely. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor option buttons now reveal the inputs, the response is recorded into current_review (was lost before), the field is colored/stated, the description resolves, and prior in-session responses restore. Mirrors the reviewer; dedup is a Phase 3 task. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Fixes the long-standing double-R typo in the contributor view class name. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
delete_peer_review_simple_view now delegates to the single delete_peer_review implementation, with an English docstring; drops the now-unused PeerReview import. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Badge policy is isolated behind a BadgeStrategy seam (DEFAULT_BADGE_STRATEGY / strategy= arg) so the calculation and which-fields-matter can change without touching callers. Default cumulative_tier_strategy derives tiers from the schema badge attributes; reviewer choice overrides the auto-suggestion; normalize_badge unifies platin/Platinum/PLATINUM. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
_apply_finished now resolves the final badge (reviewer choice else auto-suggest) from the merged metadata and persists it to metadata['review']['badge'] on both the live table and the snapshot. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
It was used by both the field-description renderer and the history renderer; promoting it to utilities lets the upcoming field_history module reuse it. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Moves renderAllFieldHistories + its historyItemHtml helper out of the peer_review.js god module into a focused module (Phase 3 incremental split). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Imports escapeHtml from utilities, re-exports renderAllFieldHistories from field_history.js so existing importers (main.js) are unchanged, and drops the moved definitions. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Moves updateFieldDescription + highlightSelectedField and their fallback-text helpers out of peer_review.js into a focused module (Phase 3 incremental split). The dead normalizeFieldKey helper is dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…tion.js Imports and re-exports updateFieldDescription/highlightSelectedField so the role modules' import paths are unchanged; drops the moved code and the now-unused escapeHtml import. peer_review.js is down from 435 to 250 lines. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Moves clearInputFields and the reviewer_remarks/reviewer_comments show/hide helpers out of peer_review.js into a focused module (Phase 3 incremental split). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Imports clearInputFields (still used in initializeEventBindings) and re-exports all five toggles so the role modules' import paths are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
applyReadOnlyMode() hid .content-finish-review, which wraps both the progress circle and the category tab navigation (#myTab). In any read-only state (finished review, or contributor not-their-turn) this stripped the tabs and stranded the user on the General pane with all its content stacked. Hide only the progress circle (.ok-progress-wrapper) instead, keeping the tabs navigable. Affects both reviewer and contributor pages. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
group_index_only() returns each general group as a flat list of items, but the template iterated group.flat (a dict key) — which resolves to nothing on a list, so every general group rendered as a header with an empty body. Iterate the list directly. Also drops the dead inner-accordion block (general groups never nest a second level). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The reviewer page shows only meta.general.flat (its grouped-meta branch is never taken), so the grouped general accordions (review/schema/topics/…) never appear there. The contributor rendered those accordions from group_index_only's list output, producing empty/odd accordions. Drop them so both pages render the General tab identically. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Replaces the nested tabs + tables + nested accordions + duplicate 'views' tab with a single scannable layout: a sticky overview bar showing per-state counts with colored dots, then one section per category listing each reviewed field as a row with a colored status dot (green=accepted, yellow=suggested, red=rejected, hollow=to review), its value, the proposed value for suggestions, and the comment. Colors match the field-state borders (theme green/yellow/red). Data gathering is unchanged; only the rendering was rewritten. Styles are injected once from JS since the widget is shared by both the reviewer and contributor pages. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Field names carry zero-based array indices (e.g. 'licenses 0 path'). Render numeric tokens as a 1-based '#N' marker so list elements read naturally ('licenses #1 path').
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Group the flat module pile by concern for navigability: core/ peer_review.js, state_current_review.js, selectors.js, store.js roles/ opr_reviewer.js, opr_reviewer_logic.js, opr_contributor.js ui/ navigation.js, summary.js, field_description.js, field_history.js, input_toggles.js data/ schema.json, example_datenmodell.json, template_datenmodell.json __tests__/ *.test.js main.js, api.js, utilities.js stay at the root. All relative imports were rewritten accordingly; main.js (the vite entry) keeps its path so the vite_asset reference is unchanged. Also updates the REUSE.toml annotation paths for the moved JSON files and the profile dashboard's opr_reviewer.js script src. No code behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Each overview stat chip (except Empty) is now a toggle: click one or more to show only those states; click again to clear. The selected set persists across the re-render after each save. Categories with no visible rows hide, and a 'No fields match this filter' note shows if a filter excludes everything. Keyboard accessible (Enter/Space, aria-pressed). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
A stray </div> closed #myTabContent right after the source pane, leaving the license and summary panes as siblings of .tab-content. Bootstrap's hide rule (.tab-content > .tab-pane) only matches direct children, so the empty license .review__items stayed visible on every tab (the 'hidden accordion' above the summary). Move the closing div so both panes nest correctly. Contributor template was already correct. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The dot logic was reviewer-only (all non-empty fields have a state), so on the contributor view every category showed green immediately because the reviewer had already set states. Now role-aware: reviewer -> green when every non-empty field has a state; contributor -> red while any reviewer-flagged (suggestion/deny) field in the category still lacks a contributor response this round, green once all are answered. Also folds the duplicate updateTabClasses into this single function (it now also adds the 'status' class so the dot renders). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
… rounds The reviewer branch used 'has any state', which is always true after round 1 (every field already carries the previous round's state), so the reviewer's dots went green even when the contributor had handed fields back. Decide instead from each field's last committed contribution in window.field_history: a field needs the current actor only if the other party's last action on it was a suggestion/deny and the actor hasn't answered this round (current_review.reviews); round 1 keeps the reviewer-reviews-everything rule. Works symmetrically for both roles every round, and handles resumed drafts (saved actions appear in field_history). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
selectFirstReviewableField picked the first unreviewed field, but from round 2 on every field already has a state, so it fell back to the first field instead of the one the contributor handed back. Use the same who-acted-last rule as the tab dots (field_history + current_review.reviews): first field where the other party's last action was a suggestion/deny and the reviewer hasn't answered this round; round 1 keeps first-unreviewed; fall back to the first field when nothing needs action. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
… Filter label The filter chips were count-gated, so on a view with no suggested/denied fields those filters disappeared. Always render the four actionable-state chips (Accepted/Suggested/Deny/To review) as filters, dimmed when their count is 0, and prefix the bar with a 'Filter' label so it reads as a control. Empty stays an informational tally. Per-row status pills (the field's state) are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…view) The gather only read suggestion/deny from current_review.reviews (this session's responses), so on the contributor page the reviewer's suggested/denied fields - which live in state_dict, not the contributor's own reviews - fell through to 'To review'. Rewrote the gather as one role-agnostic pass that classifies each field by its current state (getFieldState/state_dict, reflecting whoever acted last), sourcing the suggestion/comment text from this round's response when present, else the server-rendered field row. The contributor now sees Suggested/Deny correctly; the reviewer view is unchanged. Drops the old three-pass dedup logic. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…ixes Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of the discussion
During development for the Open Peer Review feature we found that the current architecture is to complex and hard to maintain. This PR streamlines the full OPR code in a full refactor while keeping the functionality.
Type of change (CHANGELOG.md)
Features
colored dots, comments, and clickable filters by review state.
(#2345)
Bugs
reviews, render the contributor General tab correctly, and make the category
indicator dots, summary states, and auto-select reflect each round's pending
actions for both reviewer and contributor.
(#2345)
Code Quality
ReviewService) withappend-only review rounds + projection, a shared template partial removing
reviewer/contributor duplication, and the
peer_reviewJS reorganized intocore/roles/ui/modules.(#2345)
Documentation updates
(#)
Workflow checklist
Automation
Closes #2272
Part of #2271
PR-Assignee
CONTRIBUTING.md
CHANGELOG.md
mkdocs
Reviewer
Reviewer Guidelines