Refactor image server uri helpers #19271
Merged
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
Jenkins BuildsClick to see older builds (38)
|
clauxx
reviewed
Mar 15, 2024
seanstrom
force-pushed
the
seanstrom/refactor-image-server-uri-helpers
branch
2 times, most recently
from
b9a2657 to
1362a09
Compare
seanstrom
force-pushed
the
seanstrom/refactor-image-server-uri-helpers
branch
from
89f6106 to
7ac36c5
Compare
|
hey @seanstrom still relevant? |
|
@flexsurfer yup yup it should be still relevant, but I have to handle some rebase conflicts. |
seanstrom
force-pushed
the
seanstrom/refactor-image-server-uri-helpers
branch
from
7ac36c5 to
94ccc58
Compare
seanstrom
force-pushed
the
seanstrom/refactor-image-server-uri-helpers
branch
from
94ccc58 to
d2184db
Compare
seanstrom
changed the title
shivekkhurana
approved these changes
May 2, 2024
flexsurfer
approved these changes
May 2, 2024
| :contact get-contact-image-uri | ||
| :initials get-initials-avatar-uri | ||
| str) | ||
| (-> (merge options profile-picture-options) |
There was a problem hiding this comment.
just wondering why we can't merge outside this function?
There was a problem hiding this comment.
I suppose we could merge on the outside, but at the moment merge is sorta an internal detail atm. For example, later on we could change how we extract options from each of the maps.
clauxx
approved these changes
May 7, 2024
seanstrom
force-pushed
the
seanstrom/refactor-image-server-uri-helpers
branch
from
08b65d6 to
5b50b2b
Compare
63% of end-end tests have passedFailed tests (17)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (33)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
18% of end-end tests have passedFailed tests (14)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestDeepLinksOneDevice:
Passed tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
|
mariia-skrypnyk
force-pushed
the
seanstrom/refactor-image-server-uri-helpers
branch
from
5b50b2b to
f45e5a1
Compare
|
Hi @seanstrom ! Thanks for your PR! Here is a crash I've found during iOS testing ISSUE 1: iOS crashes on Onboarding after user set his profile photo Steps:
Actual result: app crashes after Enable notifications onboarding step It happens 5/5 times |
|
@mariia-skrypnyk Nice find! 🙌 |
THX! UPD: crash exist on two last builds @seanstrom |
|
Hmm very unusual, I've tried reproducing the issue on iOS simulator, but I didn't see a crash. I'll try building and testing on device now 👍 |
|
@mariia-skrypnyk just curious, does it still crash if you do not enable notifications? |
@seanstrom found out that nightly also contains this "disease" (didn't catch it from the first try and thought that nightly is ok). No need to investigate it. Sorry for leading you down the wrong path 🙏. So, no I only need to avoid onboarding and look at all images/profile places inside the app. |
57% of end-end tests have passedFailed tests (6)Click to expandClass TestDeepLinksOneDevice:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (8)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
seanstrom
force-pushed
the
seanstrom/refactor-image-server-uri-helpers
branch
from
f45e5a1 to
a7dcedb
Compare
primary is being used as the default color for accounts that were migrated without customization color to a default color
seanstrom
force-pushed
the
seanstrom/refactor-image-server-uri-helpers
branch
from
a7dcedb to
baa23e5
Compare
|
Hi @seanstrom ! Checked main places with profile images. |
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.
fixes #18967
Summary
typekeyword. This keyword is meant to describe what kind of URI builder function should be used to build the image URI.typekeyword.Testing notes
Since this refactors an important piece of logic for rendering profile images, there should likely be a regression test of the whole app, or at least the main parts of the app that display profile images. For example:
Platforms
Areas that maybe impacted
Functional
status: ready