QuickEditor: Fix AvatarStorage.updateAvatar to not override the selected value
#527
Conversation
hamorillo
added
Bug
📲 You can test the changes from this Pull Request in Gravatar Demo by scanning the QR code below to install the corresponding build.
|
| avatarToUpdate | ||
| // If the avatarToUpdate has a selected value, use it. | ||
| // Otherwise, use the stored avatar selected value. | ||
| avatarToUpdate.copy(selected = avatarToUpdate.selected ?: avatar.selected) |
There was a problem hiding this comment.
💡 This solution works but I see a potential issue. This would stop working if backend started sending back the selcted value as false instead of null. If we only copied here the values that can actually change (rating and the alt_text) we would be much safer.
This might be unlikely scenario, but technically could happen.
WDYT?
There was a problem hiding this comment.
This would stop working if backend started sending back the selcted value as false instead of null.
In my opinion, that would be a bug on the backend because, as we've seen, we don't know if it is selected or not. If the backend sends that value as false/true, it is because that's the correct value, and we should update it.
Anyway, I'm not entirely against only updating rating and alt_text, but I was looking at the updateAvatar method as a more abstract one that, if, for example, the URL changes, can update the URL too. (Just as an example).
I can refactor this if you still feel that approach is much safer. 🙂
There was a problem hiding this comment.
In my opinion, that would be a bug on the backend because, as we've seen, we don't know if it is selected or not. If the backend sends that value as false/true, it is because that's the correct value, and we should update it.
I agree.
I can refactor this if you still feel that approach is much safer.
No need to, let's stick with this one.
…not null The PATCH to update avatars doesn't receive the email, so we can't decide if that's the selected avatar for the e-mail we are working with. In the AvatarStorage.updateAvatar method, we should use the value from the updatedAvatar only if it's not null. Otherwise, we should keep the stored avatar's selected value.
hamorillo
force-pushed
the
hamorillo/526-not-override-whole-updated-avatar
branch
from
622663a to
3037fc3
Compare
Closes #526
Description
The
PATCHto update avatars doesn't receive the email, so, in the backend, we can't decide if that's the avatar selected for the email we are working with.In the
AvatarStorage.updateAvatarmethod, we should only use the value from theupdatedAvatarif it's notnull. Otherwise, we should keep the stored avatar's selected value.Before
Kapture.2025-01-09.at.17.15.03.mp4
After
Kapture.2025-01-10.at.10.45.48.mp4
Testing Steps