I saw this on a forum today
And I think Discourse is right: There is no dashboard.problem.sidekiq_check. There are
which are referenced here:
But it seems Discourse refers to the name of that problem check instead of using the override_key.
3 Likes
Fixed via:
2 Likes
Can you help me to understand how the fix works? I see the translation key in server.en.yml and the override_key have been renamed to match the name of the check. I wonder why both needed to be renamed to match the name of the file. Shouldn’t it also have worked with sidekiq instead of sidekiq_check? I wonder if it’s still the name of the check instead of the override_key that influences which warning is shown, so I wonder if the override dashboard.problem.queue_size works or if that is a text that is never shown just like dashboard.problem.sidekiq was not.
2 Likes
Yes, we refer in a separate file to the problem check identifier:
So the PR above makes sure that the string in the translations matches the problem check filename, which is converted to that identifier, i.e. sidekiq_check. Using sidekiq in the translation strings wasn’t working, it wasn’t matching the identifier.
1 Like
Sorry, I still don’t really understand.
queue_size doesn’t match the filename/identifier either, just like sidekiq did not. And I still don’t understand why this isn’t a problem in that case.
Is this because there is another code path that directly uses the identifier instead of one of the override keys? So instead of adding a third translation key matching the filename, you changed sidekiq to use the identifier-based key? Kind of a two-in-one solution supporting both the override case and the identifier case?
If so, I don’t understand why dashboard.problem.sidekiq_check needs to be passed as an override key. The other problem checks where the translation key matches the filename don’t need that. So why is an override needed here?
1 Like
Yes, correct.
It isn’t needed, this likely would work fine if we did return problem in line 8 of sidekiq_check.rb. They’re equivalent. (I kept the override reference to have a smaller change in that PR at the time.)
1 Like
Is there something I can change easily on a dev install to trigger the dashboard.problem.queue_size error message?
I thought changing
def massive_queue?
Jobs.queued >= 100_000
end
to >= 0 would result in that error being triggered - but this resulted in dashboard.problem.sidekiq_check
1 Like
It’s easier to reproduce in system specs. I tried to do that and it exposed some more issues, this should fix them:
1 Like
For translating the Discourse interface, it’s sometimes helpful to see the text in its context, which is why I sometimes try to trigger such warnings and am always interested in learning how to do that more easily. Is that possible with system specs? Or is your “easier” only related to the context of ensuring the code works as expected?
Yes, you can use system specs to see translations in context.
If you have a dev environment for Discourse, you can do something like this:
- add a
pause_teststatement in the system spec that you want to see in a browser - run that system spec in headful mode, meaning, with a full browser (by default, specs run in headless mode)
For example, for the spec above, I added pause_test to line 47 of admin_notices_spec.rb and then ran the spec with:
PLAYWRIGHT_HEADLESS=0 bin/rspec spec/system/admin_notices_spec.rb:47
That launched a browser and paused in that specific screen:
1 Like