Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Add option to hide all paper details from conflicted PC members #366

Closed

Conversation

AGrillenberger
Copy link
Contributor

In our conferences, it is usual that PC members who have a conflict with a paper, cannot see anything about these papers (no abstract, title, PDF). As this was not possible yet with HotCRP, I implemented this by...

  • adding the setting pc_hide_conflicted_papers (yes/no)
  • adapting paper details page to hide title, abstract and so on, if applicable
  • adapting paper list / search to hide title, PDF link, abstract, if applicable
  • adapting document handler to deny access to the PDF, if applicable

@kohler
Copy link
Owner

kohler commented Oct 6, 2024

Thanks!! This is a good idea but needs a different implementation. The place to put this logic is Contact::rights and Contact::can_view_paper, in src/contact.php. In particular, I think you want to add another clause to the setting of $allow_pc_broad in Contact::rights. Something like

$allow_pc_broad = $allow_administer
  || ($isPC
      && ($ci->conflictType <= CONFLICT_MAXUNCONFLICTED || $this->conf->pc_hideconflicts));

Changes to aspects of HotCRP rights are very important to test as well.

@AGrillenberger
Copy link
Contributor Author

AGrillenberger commented Oct 6, 2024 via email

Copy link
Owner

@kohler kohler left a comment

Choose a reason for hiding this comment

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

Overall comment is that settings aside, you want to start again.

You may not be familiar with HotCRP's test suite. Run it with test/check.sh

@@ -257,6 +257,9 @@ class Conf {
const BLIND_ALWAYS = 2;
const BLIND_UNTILREVIEW = 3;

const PC_HIDECONFLICTED_NO = 0;
const PC_HIDECONFLICTED_YES = 1;

Copy link
Owner

Choose a reason for hiding this comment

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

Better to have just a boolean (no explicit constants)

if(!$user->privChair && $prow->conf->settings["pc_hideconflicted"] == 1 && $prow->has_conflict($user) && !$row->has_author($pl->user)) {
self::error("403 Forbidden", MessageItem::error("<5>Access denied to conflicting submission."), $qreq);
}

Copy link
Owner

Choose a reason for hiding this comment

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

This should not be needed. The immediately preceding logic should suffice. If you want a special error message you could implement that logic in src/failurereason.php.

$pt->print_paper_info();
if($pt->user->privChair || !($pt->conf->settings["pc_hideconflicted"] == 1 && $pt->prow->has_conflict($pt->user) && !$pt->prow->has_author($pt->user))) {
$pt->print_paper_info();
}
Copy link
Owner

Choose a reason for hiding this comment

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

Don;t want this

if(!$pl->user->privChair && $pl->conf->settings["pc_hideconflicted"] == 1 && $row->has_conflict($pl->user) && !$row->has_author($pl->user)) {
return "#{$row->paperId}";
}

Copy link
Owner

Choose a reason for hiding this comment

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

Don't want this (logic should be in can_view_paper)

if ($this->want_decoration
&& ($pl->row_tags !== "" || $pl->row_tags_override !== "")) {
$t .= $row->decoration_html($pl->user, $pl->row_tags, $pl->row_tags_override);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Don't want this (logic again should be in can_view_paper)

if($pl->conf->settings["pc_hideconflicted"] == 1 && $row->has_conflict($pl->user) && !$row->has_author($pl->user)) {
return "";
}

Copy link
Owner

Choose a reason for hiding this comment

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

Ditto

if (!$paperTable->user->privChair && $prow->has_conflict($paperTable->user) && !$prow->has_author($paperTable->user)) {
$paperConflicted = true;
}

Copy link
Owner

Choose a reason for hiding this comment

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

Ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants