Remove 3 words alias #17042
Merged
Remove 3 words alias #17042
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
J-Son89
approved these changes
Aug 17, 2023
Jenkins BuildsClick to see older builds (13)
|
cammellos
force-pushed
the
bug/remove-3-words-alias
branch
from
7fa6eaa to
3cf16bd
Compare
yqrashawn
approved these changes
Aug 17, 2023
Parveshdhull
approved these changes
Aug 17, 2023
72% of end-end tests have passedFailed tests (12)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (31)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
pavloburykh
force-pushed
the
bug/remove-3-words-alias
branch
from
3cf16bd to
585c9ee
Compare
79% of end-end tests have passedFailed tests (9)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Passed tests (34)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
@cammellos thanx for the PR!
I restored my main account which used to have a lot 3 random names contacts and I see that all of those contacts are displayed with keys (without 3 random names)
Currently when I add Desktop user as a contact I observe Display name not being fetched until CR is accepted. Tested on Desktop release version 0.13.3. telegram-cloud-document-2-5411571751152793317.mp4 |
|
Thanks for testing @pavloburykh
Yes, that's expected, anytime you saw a 3 random words username, now you see a compressed pk, looks ugly, but if we are to get rid of 3 random names, that's the only way, we should make sure we limit the time you see pks, but a fallback is inevitable.
Not quite, if they use the status-go version, they will also default to compressed pks instead of 3 random words, they just have to upgrade status go, but no work should be necessary (if they implemented things in the same way as mobile).
Is it different in this PR from develop? That code should not have changed in this PR Thank you |
@cammellos yes, it is different. I checked with mobile nightly build - Desktop display name is being fetched right after scanning QR code from mobile. |
81% of end-end tests have passedFailed tests (8)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (35)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
@cammellos ignore this for now please. I think it is randomly happening. Sometimes display name still not fetched right after scanning QR code. I believe it has nothing to do with this PR changes. |
ISSUE 1 PN does not appear for receiverE2E tests a catching this issue in all re-runs. Manually I have faced it once, but failed to reproduce after app re-install. Below I provide links to e2e video and logs Failed test https://ethstatus.testrail.net/index.php?/tests/view/3217537 Steps:
Actual result: PN does not appear for Device 1 @cammellos could you please check the above logs and tell if it will help to figure out the problem |
cammellos
force-pushed
the
bug/remove-3-words-alias
branch
from
585c9ee to
ce3cd4c
Compare
81% of end-end tests have passedFailed tests (8)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (35)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
65% of end-end tests have passedFailed tests (15)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (28)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
63% of end-end tests have passedFailed tests (16)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (27)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
hi @cammellos, thanks for you PR!
|
@yevh-berdnyk thanx for pointing out these issues. @cammellos small update:
|
@cammellos @yevh-berdnyk since we are facing point 2 in other PRs already it is not in scope of current PR and should be addressed separately. |
|
2 is reported in #17114 |
|
@cammellos hi! Could you please rebase go and mobile branches once again? I will re-run e2e and perform smoke test so we can merge this PR. As far as understand all mentioned go bugs should be already fixed. |
cammellos
force-pushed
the
bug/remove-3-words-alias
branch
from
ce3cd4c to
a41859b
Compare
|
@pavloburykh done, thank you for reminding me! |
cammellos
force-pushed
the
bug/remove-3-words-alias
branch
from
a41859b to
83f96ff
Compare
60% of end-end tests have passedFailed tests (17)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Passed tests (26)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
@cammellos thank you for the PR. Failed e2e are not PR related. Ready for merge. |
|
@cammellos oops, I have forgotten about design-review stage. Let's wait for @Francesca-G approval before merge. |
|
@pavloburykh this PR has no visual changes, other than replacing 3 words name to compressed pk, and I am not sure we ever show that in the designs (designs always show a display name), but happy for it to go through a design review of course |
Francesca-G
approved these changes
Aug 30, 2023
cammellos
force-pushed
the
bug/remove-3-words-alias
branch
from
83f96ff to
a00eb62
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.
This commit replaces the 3 random words alias with
zQ....something, as per designs.It will only work for accounts that are not in your contact yet, previous accounts will still show the 3 random words name (thought that could be fixed, but I could do in a separate PR).