Details
Manual testing with mocked invalid access token
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
getKeyserverOverrideForAnInviteLink does not appear to be handled, even though it was extracted in D13958.
I'm worried that we're on relying on diff reviewers to do a thorough analysis of all callsites, which is not an appropriate role for a reviewer.
@bartek, if I missed it, can you please link the diff where this is handled? On the other hand, if it's not handled yet, can you share how you're keeping track of each callsite that needs to be migrated?
| lib/actions/upload-actions.js | ||
|---|---|---|
| 36 ↗ | (On Diff #45920) | How was this not causing ESLint errors earlier? Something seems sketchy in this stack... saw the same thing earlier here. @bartek, can you please explain what you were doing here, and how you managed to get earlier diffs to pass ESLint even though there were clear issues like this? Perhaps you have commits in your branch that you haven't diffed up? If so, please squash those and update all of the diffs. And separately, please make sure to apply fixes like this to the correct commits in the future, instead of putting them wherever is convenient for you. |
| lib/actions/upload-actions.js | ||
|---|---|---|
| 36 ↗ | (On Diff #45920) | This was my mistake. When resolving conflicts, pre-commit hooks don't apply. I'll rebase the whole stack to make sure everything is correct now.
I'll keep that in mind, thank you |
| lib/actions/upload-actions.js | ||
|---|---|---|
| 36 ↗ | (On Diff #45920) | Thanks :) |
Rebase, handle getKeyserverOverrideForAnInviteLink better.
Initially I was thinking that this could be handled the same way as error case so the function should return null. After digging deeper into the invite links code I realized this was wrong and it should also be handled separately.
Thanks for handling getKeyserverOverrideForAnInviteLink!
Would mind sharing how you're keeping track of each callsite that needs to be migrated? I'm worried because I caught two cases in my review, but I didn't do an exhaustive analysis, and so I'm worried there are other cases I might not have caught.