Skip to content
This repository was archived by the owner on Oct 22, 2024. It is now read-only.

fix: close connection when pairing key does not match#270

Merged
vinistevam merged 5 commits intomain-desktopfrom
fix/212-fix-ui-after-pairing
Dec 2, 2022
Merged

fix: close connection when pairing key does not match#270
vinistevam merged 5 commits intomain-desktopfrom
fix/212-fix-ui-after-pairing

Conversation

@vinistevam
Copy link
Copy Markdown
Contributor

@vinistevam vinistevam commented Nov 23, 2022

Overview

Bug 1: whenever disabled the pairingKeyHash was not been clean

Changes

  • Included pairingKeyHash to set status on disable
  • Added PairingKeyStatus to facilitate error screen ticket: 135

Issues

Resolves #212

@vinistevam vinistevam force-pushed the fix/212-fix-ui-after-pairing branch 2 times, most recently from 4ee838b to 86c215f Compare November 23, 2022 22:09
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are there 2 versions of the state? Which one is considered as the source of truth?

Copy link
Copy Markdown
Member

@OGPoyraz OGPoyraz Nov 30, 2022

Choose a reason for hiding this comment

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

I also couldn't find the case for this extra check. Is it possible to have a discrepancy between the two states?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Last week this.desktopState.desktopEnabled was returning false while establishing a new connection, but after rebasing today I removed this change and the fix still works.

Copy link
Copy Markdown
Member

@OGPoyraz OGPoyraz left a comment

Choose a reason for hiding this comment

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

Not sure that check is needed raised by Gauthier. Apart from that it's LGTM 🚀🚀🚀

@vinistevam vinistevam force-pushed the fix/212-fix-ui-after-pairing branch 3 times, most recently from 498a09e to 5a32d49 Compare November 30, 2022 17:04
@vinistevam vinistevam marked this pull request as ready for review November 30, 2022 18:30
@vinistevam vinistevam requested a review from a team November 30, 2022 18:30
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need the alternate states here? Would no key and the wrong key not both result in the same "app not recognised" error?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is another scenario where "Desktop has no pairing key" here

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We really shouldn't be relying on implicit behaviour of enums where the values are assigned to numbers by default which in turn evaluate to truthy. We could change the order of the values, or assign them to strings and this would break.

I'd recommend assigning to a variable then doing a [PairingKeyStatus.NO_MATCH, PairingKeyStatus.MISSING].includes(pairingKeyStatus) to be explicit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! It shouldn't be like that.
I was doing negatively compared with PairingKeyStatus.MATCH but the way you recommended it's more explicit.
Pushing the changes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we need this enum, then pairingKey is implied and we've been using capitals for enums so maybe MATCH, NO_MATCH and MISSING?

@vinistevam vinistevam force-pushed the fix/212-fix-ui-after-pairing branch from 5a32d49 to 09b5020 Compare December 1, 2022 13:19
@vinistevam vinistevam force-pushed the fix/212-fix-ui-after-pairing branch from 09b5020 to a1fd484 Compare December 1, 2022 13:48
@vinistevam vinistevam merged commit aa9f8eb into main-desktop Dec 2, 2022
@vinistevam vinistevam deleted the fix/212-fix-ui-after-pairing branch December 2, 2022 11:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix bug raised by Antonela & Goktug (see description attached)

6 participants