[#15944] Set installation name on account creation and pairing #16307
+16
−19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rasom
force-pushed
the
fix/#15944-set-installation-name-android
branch
from
38b89e5 to
b05e580
Compare
Jenkins BuildsClick to see older builds (74)
|
rasom
force-pushed
the
fix/#15944-set-installation-name-android
branch
from
b05e580 to
7303792
Compare
rasom
force-pushed
the
fix/#15944-set-installation-name-android
branch
from
7303792 to
f8a328d
Compare
|
@rasom I think it would be best if we set it on account creation (less RPC calls and less work for the clients) https://github.com/status-im/status-go/blob/develop/protocol/requests/create_account.go |
J-Son89
reviewed
Jun 19, 2023
J-Son89
approved these changes
Jun 19, 2023
|
@cammellos one PR is always better than two, but if you insist... :D |
I think it's better to have it in the account creation RPC, yes, it's 2 PRs that's true, but it keeps the logic together and avoid the extra RPC, so I think it's worth it :) |
rasom
force-pushed
the
fix/#15944-set-installation-name-android
branch
from
f8a328d to
333abd0
Compare
rasom
changed the title
|
ok so... currently it is fixed on android on a regular account creation. Now I need to figure how to pass that device name during pairing process |
rasom
force-pushed
the
fix/#15944-set-installation-name-android
branch
from
e2f249a to
547f749
Compare
|
@rasom ping QA when it is ready to be tested, thank you! |
rasom
force-pushed
the
fix/#15944-set-installation-name-android
branch
6 times, most recently
from
d43af12 to
3cd02c9
Compare
|
@churik should be ready now |
70% of end-end tests have passedFailed tests (10)Click to expandClass TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Passed tests (23)Click to expandClass TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
|
rasom
changed the title
|
Hi @rasom, thank you for the PR. The PR looks good. I just have a few questions: In the device settings, there is a "Device Name" value. However, I noticed that currently, the value is only fetched on the 'sync complete' page for iOS devices. For Android devices, the name of the model is fetched instead of the "Device Name" value. I wanted to confirm if this is the expected behavior, or if for Android devices, the "Device Name" value should also be fetched. Could you please clarify? QESTION 1: [Android] The model name is fetched instead of the device nameSteps to reproduce:
Actual result:The value from the 'Device name' is fetched on the Sync page how it works on Android Expected result:The value from the 'Device name' is fetched on the Sync page for Android ??? how it works on iPhone |
Question 2 Should the name of the non-paired device be shown on the 'Syncing' page. If yes, should it be included in the scope of the current PR?Steps to reproduce:
Actual result:The Expected result:The |
Question 3: Should the online status and device type (mobile/laptop) indicators be included in the scope of the current PR?Actual result:
Expected result: |
|
@VladimrLitvinenko q1. there is no reliable way to get that device name on android, it kind of works differently from device to device. As far as we get at least some name which is not |
@rasom Thanks for the clarification. I have no issues from my side. I noticed a new conflict has appeared. Please let me know if, after resolving the conflict, the PR needs to be retested. I can quickly retest it. Otherwise, the PR can be merged |
rasom
force-pushed
the
fix/#15944-set-installation-name-android
branch
from
5b4e594 to
d0f5971
Compare
rasom
force-pushed
the
fix/#15944-set-installation-name-android
branch
2 times, most recently
from
8fcdb67 to
0466a5b
Compare
rasom
force-pushed
the
fix/#15944-set-installation-name-android
branch
from
0466a5b to
b953ddb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix #15944
This line returns "localhost" https://github.com/status-im/status-go/blob/develop/server/device.go#L29, so instead we set installation name during account creation or pairing.
status: ready