Skip to content
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

Native lookup fixes #6101

Merged
merged 5 commits into from
Jun 28, 2022
Merged

Native lookup fixes #6101

merged 5 commits into from
Jun 28, 2022

Conversation

chagai95
Copy link
Contributor

@chagai95 chagai95 commented May 19, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

refactor - better naming, return native user id and not sip user id and create a dm with the native user instead of with the sip user

Motivation and context

When dialing an extension which has a native user behind it, a native matrix call happens instead of a matrix sip call because the user id of the sip user was used instead of the native one - so the sip virtual lookup (which comes in this case after the native lookup) failed. Otherwise also creating a native room when one does not exist seems like the right thing to do and the code looks more clear with these names.

Screenshots / GIFs

Tests

  • private setup - you should have access in Aarenet/Element room

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

Signed-off-by: Chagai Friedlander @me:chagai.website

…nd create a dm with the native user instead of with the sip user
@bmarty bmarty added the Z-Community-PR Issue is solved by a community member's PR label May 20, 2022
Copy link
Member

@bmarty bmarty 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 the PR.
LGTM, but I will defer to @ganfra and/or @dbkr for the logic.

@bmarty
Copy link
Member

bmarty commented May 20, 2022

(can you check the compilation issues please?)

@bmarty bmarty requested review from dbkr and ganfra May 20, 2022 09:58
@chagai95
Copy link
Contributor Author

(can you check the compilation issues please?)

Couldn't figure out where the actual errors are... can you help me with that?

} else {
// do the same if there is no corresponding native user.
directRoomHelper.ensureDMExists(sipUserId)
var nativeRoomId = session.getExistingDirectRoomWithUser(nativeUserId)
Copy link
Member

Choose a reason for hiding this comment

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

You are maybe not using the latest develop from mainline?
The code should be session .roomService() .getExistingDirectRoomWithUser(nativeUserId) now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, completely missed that - copy pasted that part of the change from our fork, sorry.

// just create a DM with the native user
nativeRoomId = directRoomHelper.ensureDMExists(nativeUserId)
}
Timber.d("lookupPhoneNumber with nativeUserId: $nativeUserId and nativeRoomId: $nativeRoomId")
Copy link
Member

Choose a reason for hiding this comment

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

An import of Timber is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, sorry 🙈

@bmarty
Copy link
Member

bmarty commented May 20, 2022

(can you check the compilation issues please?)

Couldn't figure out where the actual errors are... can you help me with that?

I have added some comments and the error details can be found on clicking on "Details" (in case you are not aware of that):

image

@bmarty
Copy link
Member

bmarty commented May 20, 2022

(it's confirmed I am not the only one to create PR without even having compiled the code locally!)

@chagai95
Copy link
Contributor Author

(can you check the compilation issues please?)

Couldn't figure out where the actual errors are... can you help me with that?

I have added some comments and the error details can be found on clicking on "Details" (in case you are not aware of that):

image

I tried that, but still couldn't really figure out where the errors are... sorry... also sorry for not compiling locally 🙈
Here is a screen cap of me trying to understand the failing builds:
https://aarenet-my.sharepoint.com/:v:/p/chagai_friedlander/EcN-uVEqkIxGkfvg3lPYKVQBGVsf0T_6BMwpUkVH7VGilA

@langleyd langleyd self-requested a review May 31, 2022 11:34
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

The (pre-existing) variable names in here are quite confusing, but the logic of the change looks correct.

@chagai95
Copy link
Contributor Author

I think everything passed?

* What went wrong:
Execution failed for task ':sonarqube'.
> Project not found. Please check the 'sonar.projectKey' and 'sonar.organization' properties, the 'SONAR_TOKEN' environment variable, or contact the project administrator

@chagai95 chagai95 requested a review from bmarty June 17, 2022 05:55
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

The error reported by the CI can be ignored; we will try to fix that.
Thanks for the update, and thanks @dbkr for the review!

@bmarty bmarty merged commit 6fda2cc into element-hq:develop Jun 28, 2022
@chagai95 chagai95 deleted the native-lookup-fixes branch June 28, 2022 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants