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

Fix an issue with avatars not loading in suggestions #21169

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

kean
Copy link
Contributor

@kean kean commented Jul 21, 2023

Fixes #21155

The reason it's not working is the following change to the avatar hashes: pb6Nl-gyD-p2.

The proper fix will require a little more work. The app shouldn't make any assumptions about the contents of these identifiers. It should also not use WPAvatarSource because it's yet another non-centralized image loading system in the app with its own cache, which contributes to high memory usage issues, and it fails to reuse the caches and there work for fetching the same avatars in other places of the app.

To test:

  • Open Reader
  • Open one of the A8C posts
  • Tap to reply and start typing a user name (staring with @ sign)
  • Verify that the avatars are loaded

Regression Notes

  1. Potential unintended areas of impact: user suggestions list in reader replies
  2. What I did to test those areas of impact (or what existing automated tests I relied on): manual testing
  3. What automated tests I added (or what prevented me from doing so): n/a

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@kean kean requested review from guarani and salimbraksa July 21, 2023 16:26
@kean kean added this to the Pending milestone Jul 21, 2023
@wpmobilebot
Copy link
Contributor

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr21169-49eaecc
Version22.8
Bundle IDcom.jetpack.alpha
Commit49eaecc
App Center Buildjetpack-installable-builds #5500
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr21169-49eaecc
Version22.8
Bundle IDorg.wordpress.alpha
Commit49eaecc
App Center BuildWPiOS - One-Offs #6469
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

Change looks good! Tested and verified avatars now work. I didn't test blavatars though, but it seems like they work very similarly.

@@ -129,7 +129,7 @@ - (WPAvatarSourceType)parseURL:(NSURL *)url forAvatarHash:(NSString **)avatarHas
if ([components count] > 2) {
NSString *type = components[1];
NSString *hash = components[2];
if ([hash length] == 32) {
if ([hash length] == 32 || [hash length] == 64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense given the hash is now 64 characters 👍
nitpick: I don't know why this has to verify the hash's length, since it seems unlikely we'll get the wrong length and if we do, I don't think there's harm in it. What will happen AFAIK is that we'll just request the avatar/blavatar and it'll fail.

@guarani
Copy link
Contributor

guarani commented Jul 21, 2023

This is a hotfix. The reason it's not working is the following change to the avatar hashes: pb6Nl-gyD-p2.

If this targets trunk, it won't be a hotfix nor a betafix but just a regular fix, as defined here PbMoDN-uy-p2.

@kean kean merged commit 7271966 into trunk Jul 21, 2023
@kean kean deleted the task/fix-avatars-not-loading-in-suggestions branch July 21, 2023 20:08
@guarani guarani mentioned this pull request Jul 21, 2023
13 tasks
@kean kean modified the milestones: Pending, 22.9 ❄️ Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The avatars in users suggestions list not loading
3 participants