Approve Avatars removes pictures when used simultaneously

[ohexter]

Describe the bug If two WRT members approve the same avatar for a user the second approval removes the avatar (set to NULL). This can occur when WRT members are both working at the same time and open the same pending avatars for approval.

To Reproduce Steps to reproduce the behaviour:

1. A user U submits a new avatar
2. WRT member A opens Approve Avatars, displaying the pending avatar for U
3. WRT member B opens Approve Avatars, also displaying the pending avatar for U
4. WRT member A approves the avatar for U [following this stage the new avatar is correctly displayed]
5. WRT member B approves the avatar for U [following this stage the avatar is set to NULL]

Expected behavior Step 5 should retain the avatar submitted in 1 and approved in 4, and not set to NULL

Desktop (please complete the following information):

• OS: Windows 10
• Browser Chrome
• Version 108.0.5359.125

[dunkOnIT]

Interesting bug - my guess without looking at the code would be that "Approve" functionality is a toggle, instead of setting the state to "Approve"? Alternatively triggering the same function twice could error out which results in the NULL.

First step of working on this would be to look into the logic and understand why this is happening, and then suggest a change.

[diogojs]

Taking a look at the code, when the user submits a new avatar it's stored on pending_avatar variable/field, the function that handles the approving only updates the avatar variable with whatever is on pending_avatar (and sets the latter to nil). So when the second admin approves it, the pending_avatar is already nil and the avatar is set to nil.

def approve_pending_avatar!
    self.update_columns(
      avatar: self.read_attribute(:pending_avatar),
      saved_avatar_crop_x: self.saved_pending_avatar_crop_x, saved_avatar_crop_y: self.saved_pending_avatar_crop_y, saved_avatar_crop_w: self.saved_pending_avatar_crop_w, saved_avatar_crop_h: self.saved_pending_avatar_crop_h,
      pending_avatar: nil,
      saved_pending_avatar_crop_x: nil, saved_pending_avatar_crop_y: nil, saved_pending_avatar_crop_w: nil, saved_pending_avatar_crop_h: nil
    )
  end

A simple change would be to check if the pending_avatar is already nil at this point, then just skip/return (possibly showing a warning message to the 2nd admin?). If it's OK someone can assign this issue for me and I'll open a pull request. Ruby and Rails are not my expertise, but I would like to contribute.

[dunkOnIT]

Thanks a lot @diogojs ! Assigned to you :+1: