search

status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.9k stars 988 forks source link

"Remove current photo" is shown for new account and profile info is duplicated in header after any change #9234

Closed churik closed 4 years ago

churik commented 5 years ago

Bug Report

Problem

Expected behavior

  1. Removing photo is not shown when photo is not set
  2. The header is not changed after removing/setting custom photo

If you scrolling and switching between profile and other tabs.

Actual behavior

removing_photo

Reproduction

Additional Information

churik commented 5 years ago

@rachelhamlin issue doesn't block user from anything, but it is common action for a majority of users. So added in v1 (remove if you don't think it is important for v1) I believe can be bounty.

errorists commented 5 years ago

🙌 I'm also experiencing the duplicate header more often than I'd like, (seen above on the video), would really appreciate if there's a fix for that.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 80.0 DAI (80.0 USD @ $1.0/DAI) attached to it.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 2 weeks, 6 days from now. Please review their action plans below:

1) acolytec3 has been approved to start work.

Identify source of bug, likely not updating a subscription or db field correctly when setting the profile picture, and code appropriate fix

Learn more on the Gitcoin Issue Details page.

acolytec3 commented 5 years ago

@churik @yenda @flexsurfer @errorists Not sure who to direct this question to but in looking at the underlying code, is it safe to assume that if a user ever adds an image as their profile picture, it will always be jpeg and if it's the system generated identicon, it will always be png? It looks like the update-picture event assumes any user-provided picture will be jpeg. I'm hoping this is the case as it should make a fix relatively straight-forward in terms of adjusting the display rules for the above bug can be guided by inferring whether to display the "Remove photo" option based on whether the photo-path is jpeg or png.

errorists commented 5 years ago

To me it’s not as clear, I have pictures in my gallery that aren’t JPG and I can choose to use any one of them as my profile pic. Unless we save these locally as jpegs, chances are you’d end up using PNGs for avatar pictures too.

acolytec3 commented 5 years ago

@errorists That's what I wondered too but when I set an image with a PNG extension as my profile pic, re-frisk is still showing the photo-path value as a base64/jpeg. I think that regardless of the actual format, it's being saved as a bas64/jpeg representation by update-picture and that seems to work just fine. So, I think should be a safe assumption unless somebody else knows otherwise. I'll get a PR started tonight or first thing tomorrow.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 80.0 DAI (80.0 USD @ $1.0/DAI) has been submitted by:

  1. @acolytec3

@StatusSceptre please take a look at the submitted work:

bitsikka commented 5 years ago

@errorists That's what I wondered too but when I set an image with a PNG extension as my profile pic, re-frisk is still showing the photo-path value as a base64/jpeg. I think that regardless of the actual format, it's being saved as a bas64/jpeg representation by update-picture and that seems to work just fine. So, I think should be a safe assumption unless somebody else knows otherwise. I'll get a PR started tonight or first thing tomorrow.

@acolytec3

I have worked in these areas and have few notes I'd like to share

I too have used profile pic being saved as base64/jpeg to determine whether or not to render border around profile pic.

So that approach also works.

Still, fwiw, In the past, it worked as intended, with a little bit different implementation (and so this bug is a regression).

The logic used to be, to compare photo-path with string returned by identicon/identicon function.

in status-im.ui.screens.profile.user.views

(defn- header [{:keys [public-key photo-path] :as account}]
  [profile.components/profile-header
   {:contact                account
    :allow-icon-change?     true
    :include-remove-action? (not= (identicon/identicon public-key) photo-path)}])
acolytec3 commented 5 years ago

@bitsikka I like that approach. Solves the issue more exactly than mine does. That said, it feels like execution time might be slightly longer if you're generating an identicon every time you render the page versus just looking at the first part of the string. I dunno if it's enough to make a difference in page load so will leave it to the reviewers on whether to change. Thanks for the suggestion!

bitsikka commented 5 years ago

@bitsikka I like that approach. Solves the issue more exactly than mine does. That said, it feels like execution time might be slightly longer if you're generating an identicon every time you render the page versus just looking at the first part of the string. I dunno if it's enough to make a difference in page load so will leave it to the reviewers on whether to change. Thanks for the suggestion!

@acolytec3

đź‘Ť

good point about perf.. Something worth looking into maybe: recently, there was a change(related to perf - moving to golang) in the way identicons are generated(and maybe cached - not sure)

Edit: confirmed! identicon/identicon is memoized (plus, one/first time execution done in status-go) - pretty sure ok to use perf wise

yenda commented 5 years ago

@bitsikka @acolytec3 I think checking if the photo-path is the same as the identicon is the most robust solution. Also there is no point doing that check more than once. You can do it at login, and store some value in app-db like personalized-picture? that you set to true if the two are different.

bitsikka commented 5 years ago

@bitsikka @acolytec3 I think checking if the photo-path is the same as the identicon is the most robust solution. Also there is no point doing that check more than once. You can do it at login, and store some value in app-db like personalized-picture? that you set to true if the two are different.

@yenda indeed the most robust.. đź‘Ť I'm still catching up with updated changes :) and somewhat stuck behind in old implementations. I understand you mean that both identicon and photo-path fields are there in the new db, whereas iirc only photo-path was there before. Or, something like that.

acolytec3 commented 5 years ago

@yenda I'll update my PR along those lines.

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 80.0 DAI (80.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @acolytec3.