-
Notifications
You must be signed in to change notification settings - Fork 176
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
Fixing the "This competition's results are visible but not posted" #9706
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution Zain, left couple of comments. Regarding the test failure, there is a unit-test failure.
config/locales/en.yml
Outdated
@@ -1339,6 +1339,7 @@ en: | |||
uncancel_success: "Successfully uncancelled competition!" | |||
uncancel_failure: "Cannot uncancel the competition." | |||
results_not_posted: "This competition's results are visible but haven't been posted yet." | |||
results_not_posted_non_admins: "We are busy processing this competition's results - they should be available shortly." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather name the key as something like results_processing
or something better. results_processing
also looks bit bad to me, but maybe other can suggest better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregorbg do you have any suggestions for the key name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest results_still_processing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gregor's plate is pretty full, hopefully he's fine for Daniel and I to resolve this :D
app/models/competition.rb
Outdated
@@ -469,7 +469,11 @@ def warnings_for(user) | |||
end | |||
|
|||
if self.results.any? && !self.results_posted? | |||
warnings[:results] = I18n.t('competitions.messages.results_not_posted') | |||
if user.can_admin_results? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cursory look suggests that this should work, although it's causing a test to fail. I can try debug it in the next day or so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@danieljames-dj would you be able to review/approve as well? I think it's straightforward enough to merge without Gregor's approval, but would appreciate a second set of eyes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as well :-)
I'll go ahead and merge this as the change looks pretty safe for me.
This pr should solve #9229 , hopefully.
@danieljames-dj @dunkOnIT this might have errors so please review it :)