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

Chat feature: enable chat in the venue request form #2029

Merged
merged 44 commits into from
May 8, 2024

Conversation

melisabok
Copy link
Member

@melisabok melisabok commented Feb 22, 2024

When the comment stage is enabled and the chat interface is enabled for all the selected participants that are members of the reviewing committee: SACs, ACs and reviewers.

  • Chat invitation is created for each submission with the same start and end date of the official comments
  • Chat reaction tag invitation is created for each submission in order to react to the chat replies
  • Submission invitation is updated to two tabs are not shown: one for general discussion and another for the chat discussion
  • Email notifications are sent, 1 email every 5 new comments or 1 email every 4 hours
  • PCs can disable the chat functionality and keep the official comments open.

@zbialecki
Copy link
Contributor

@melisabok can you share the invitation configuration you were using with items that was causing the error?

@melisabok
Copy link
Member Author

@zbialecki I pushed a change using items, I kept the line to use enum in case you need it.

if self.Readers.REVIEWERS_ASSIGNED in self.invitees:
invitees.append(conference.get_reviewers_id(number))

if self.Readers.REVIEWERS_SUBMITTED in self.invitees:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need both Reviewers and Reviewers/Submitted right?

Copy link
Member Author

Choose a reason for hiding this comment

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

it depends on which participants they choose in the CommentStage.

Copy link
Member Author

Choose a reason for hiding this comment

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

but Reviewers/Submitted could be an option.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, it makes sense that Reviewers/Submitted could be an option, but both Reviewers and Reviewers/Submitted shouldn't be in readers at the same time. It's confusing in the UI if it says the chat is visible to Reviewers and Reviewers/Submitted

@zbialecki
Copy link
Contributor

Some issues I'm seeing with the tests:

  • when the ICML tests finishes there are 3 tabs again, including an "All" tab
  • If I'm logged in as a reviewer I can't delete my own chats

@@ -4484,8 +4484,9 @@ def test_meta_review_agreement(self, client, openreview_client, helpers, seleniu
invitations_container = selenium.find_element(By.CLASS_NAME, 'invitations-container')
invitation_buttons = invitations_container.find_element(By.CLASS_NAME, 'invitation-buttons')
buttons = invitation_buttons.find_elements(By.TAG_NAME, 'button')
assert len(buttons) == 1
assert len(buttons) == 2
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should actually be changed back - we decided that the forum page will default to the discussion tab, so the chat button will be hidden.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I changed it to make the tests pass

)
)

emoji_chat_invitation = self.venue.get_invitation_id('Chat_Emoji')
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of calling the invitation "Chat_Emoji" can we call it something more generic like "Chat_Reaction"

melisabok and others added 4 commits May 7, 2024 14:32
Co-authored-by: celestemartinez <32438984+celestemartinez@users.noreply.github.com>
Co-authored-by: celestemartinez <32438984+celestemartinez@users.noreply.github.com>
Co-authored-by: celestemartinez <32438984+celestemartinez@users.noreply.github.com>
@celestemartinez
Copy link
Member

@melisabok I think the tests need to be changed too here: comittee --> committee

@melisabok
Copy link
Member Author

ok, I will change it

Co-authored-by: carlosmondra <carlos@openreview.net>
@zbialecki
Copy link
Contributor

I tested disabling and enabling the chat via the comment stage and it's working.

@carlosmondra
Copy link
Member

Email notifications are sent, 1 email every 5 new comments or 1 email every 4 hours

What happens if there are no new comments after 4 hours? Is an email still sent?

I also think that it's not necessary to send 1 email every 5 comments. I think that sending 1 email every 4 hours if there are any new messages is enough.

@melisabok
Copy link
Member Author

What happens if there are no new comments after 4 hours? Is an email still sent?

no emails are sent if there are no new comments since the last_notified_comment_id.

@melisabok
Copy link
Member Author

I also think that it's not necessary to send 1 email every 5 comments. I think that sending 1 email every 4 hours if there are any new messages is enough.

I'm worried that could be too many messages in 4 hours and the participants won't get any notifications.

Copy link
Member

@carlosmondra carlosmondra left a comment

Choose a reason for hiding this comment

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

This is very cool!

Copy link
Member

@enrubio enrubio left a comment

Choose a reason for hiding this comment

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

Very cool!

@melisabok melisabok merged commit c76623f into master May 8, 2024
1 check passed
@melisabok melisabok deleted the feature/enable-chat-comment-stage branch May 8, 2024 15:38
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.

None yet

9 participants