Skip to content

Fix not setting database as selected if it already exists #2384

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

Merged
merged 6 commits into from
Apr 27, 2023

Conversation

robertbrignull
Copy link
Contributor

This PR should fix a bug where we aren't able to set a database as the selected database. There are two bugs which are each independently causing this behaviour:

  • In addExistingDatabaseItem if we find that a database item with that URI exists already then we return immediately, without checking makeSelected and therefore without setting the database as selected.
  • In handleSetCurrentDatabase and chooseAndSetDatabase we weren't setting makeSelected to true.

This PR hopefully addresses both points. We now check makeSelected in both branches in addExistingDatabaseItem. We also change the default value of makeSelected to true, because we believe this is always the desired behaviour, except in one case in the test runner.

I've structured the commits so that 92f23d1 where we change the default value should be 100% behaviour preserving. I hope that makes it easier to review, but if not then just ignore the commits and review the PR as a whole.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@robertbrignull robertbrignull requested review from a team as code owners April 27, 2023 14:27
Copy link
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! 🕵🏽‍♂️ I've double-checked the behaviour locally and it looks good 🎉

Since it's a user-visible bug fix, this probably deserves a note in the changelog. (Something like "Fix bug where the CodeQL: Set Current Database command didn't always select the database"? 🔍)

@shati-patel
Copy link
Contributor

The test failure looks legitimate too: we now call resolveDatabaseContents twice: once in createDatabaseItem, and once in setCurrentDatabaseItem (as part of item.refresh).

@robertbrignull
Copy link
Contributor Author

I've also managed to reproduce the bug locally and verified the fix

@robertbrignull robertbrignull merged commit 686d874 into main Apr 27, 2023
@robertbrignull robertbrignull deleted the robertbrignull/makeSelected branch April 27, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants