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

Fix/SAC console - remove hardcoded role names #1947

Merged
merged 10 commits into from
Jun 6, 2024

Conversation

xkopenreview
Copy link
Collaborator

this pr should remove hardcoded role names from sac console

@xkopenreview
Copy link
Collaborator Author

@enrubio I think this pr require more tests
it will also help you to understand how data is loaded in sac console which is about the same as in pc console so it will help when you change pc console

@enrubio
Copy link
Member

enrubio commented May 29, 2024

that would be great, thanks Xukun!

@enrubio
Copy link
Member

enrubio commented May 30, 2024

I highlighted a few places in the SAC console that need to be updated:

SAC console Submission Status tab:
Screenshot 2024-05-30 at 12 59 15 PM

Capitalization in Query Search modal for Paper Status tab used in SAC & PC console:
QuerySearchPaperStatus

Everything else looks good, SPC Status tab:
Screenshot 2024-05-30 at 1 02 53 PM

Tasks tab:
Screenshot 2024-05-30 at 1 02 58 PM

@xkopenreview
Copy link
Collaborator Author

I highlighted a few places in the SAC console that need to be updated:

SAC console Submission Status tab: Screenshot 2024-05-30 at 12 59 15 PM

Capitalization in Query Search modal for Paper Status tab used in SAC & PC console: QuerySearchPaperStatus

Everything else looks good, SPC Status tab: Screenshot 2024-05-30 at 1 02 53 PM

Tasks tab: Screenshot 2024-05-30 at 1 02 58 PM

please take a look at the changes in #1924 before you make the change here
some shared components may have been updated

@enrubio
Copy link
Member

enrubio commented May 30, 2024

Oohh okay I'll check.

I also noticed as an AC sending messages to PCs (SACs to Reviewers), the message modal correctly shows the number of emails and who they're sent to. But this was an issue in #1924 for some reason, so maybe an API change is not needed? What do you think @xkopenreview?

Screenshot 2024-05-30 at 1 15 47 PM

@enrubio
Copy link
Member

enrubio commented Jun 4, 2024

@xkopenreview Discussed with @melisabok and decided that the SAC console tests will be added in a separate PR. Right now the plan is to:

  1. Test again & merge Fix/ AC Console - remove hardcoded role names #1924. I believe we'll make a quick fix in the API to fix the email grouping issue.
  2. Review this PR again & merge it.
  3. Merge openreview-py#2109 which will add seniorAreaChairName to the PC console webfields.

@melisabok melisabok marked this pull request as ready for review June 6, 2024 17:40
@xkopenreview
Copy link
Collaborator Author

xkopenreview commented Jun 6, 2024

for those in the missingConfig of sac console
reviewerName
anonReviewerName
officalReviewName

if they are missing the sac console will not load and the menu bar won't be displayed
so i think for these it's not necessary to add a default value in the menu bar component
instead the test should be modified to add these configs

for the rest i think default value should be added to all sub components where it's used

@enrubio
Copy link
Member

enrubio commented Jun 6, 2024

I didn't end up adding any fields to the missingconfigs of the SAC or PC console, but I removed officialMetaReviewName in the missingconfig of the PC console since I added a default value.

@xkopenreview
Copy link
Collaborator Author

I didn't end up adding any fields to the missingconfigs of the SAC or PC console, but I removed officialMetaReviewName in the missingconfig of the PC console since I added a default value.

i think it will be clearer when pc console is also changed
we can run another round of testing when pc console is done

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.

Looks like everything is working as expected.

@melisabok melisabok merged commit 014e61c into master Jun 6, 2024
2 checks passed
@melisabok melisabok deleted the fix/sac-console-remove-role-names branch June 6, 2024 20:07
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.

3 participants