Closed crccheck closed 7 years ago
I have to look at code but did we build the quiz bot with the assuming that if it got a profile that said profile would have certain items? I'd worry that now if an empty item is returned, another error will bubble up.
If someone was relying on this obscure code blowing up, that should be fixed on their side
Agreed, the above is more about whether we need a task on the quiz bot repo to modify how things work if this dependency is updated. I do not think that is blocking this PR though given your above statement from the description.
I don't think quizbot and greeting message code really needs to be updated. The profile only isn't updated in rare instances. Though I'm pretty sure I've seen it fail because of network failures before. The emitOptionalEvents()
block has a check, but it's incomplete for the fallback. I'll update that before merging.
Why are we doing this?
The bot tries to get a profile, but it's a convenience, not a requirement. If for some reason the profile can't be loaded, it should not be a show stopper. There are already cases where we can't get a profile (bot messing a bot) where we fallback to an empty object and log a warning.
This PR also implements a longstanding related TODO and fixes the fallback case for the greeting message.
Did you document your work?
No README updates necessary. Code comments and type defs are updated.
How can someone test these changes?
Steps to manually verify the change:
npm t
What possible risks or adverse effects are there?
What are the follow-up tasks?
Are there any known issues?
none
Did the test coverage decrease?
increases, the profile fetching logic inside
routeEachMessage
wasn't covered before.