Skip to content

Updating a user via API sets user profile to public if private_profile field is not provided

We've identified an issue while synchronising users with our internal directory in which the user profile is always set to public if we don't provide the field in the API PUT request.

The behaviour we observe is that:

  • If the field is provided with a boolean value, it is updated correctly
  • If the field is provided with an explicit nil value, it is set to false
  • If not provided, it is updated to false, no matter which value it had

Our expectation would be that:

  • If the field is provided with a valid boolean value, it's updated with the provided value
  • If the field is provided with an explicit nil value, it is set to false
  • If not provided, it's left as it is, except if never previously set in the model, in which case it will be set to false

Initial analysis and tests show that the culprit is a default value in the API user params in lib/api/users.rb:

        params :optional_attributes do
          ...
          optional :private_profile, type: Boolean, default: false, desc: 'Flag indicating the user has a private profile'
          ...
        end

It seems unnecessary to have this default value since the user model model is taking care of it:

  before_save :default_private_profile_to_false
  ...

  def default_private_profile_to_false
    return unless private_profile_changed? && private_profile.nil?

    self.private_profile = false
  end

Is there any (historical) reason why this is the only field with a default value?

Furthermore, the specs seem to be wrong for this case in spec/requests/api/users_spec.rb since the put request will not see the updated value:

    it "updates private profile when nil is given to false" do
      # This should be `user.update...`
      admin.update(private_profile: true)

      # The next request will see `user.private_profile? == false`

      put api("/users/#{user.id}", admin), params: { private_profile: nil }

      expect(user.reload.private_profile).to eq(false)
    end

We'll be submitting an MR shortly to tackle this but wanted to give you a heads-up, since maybe there's some background reason for this behaviour.

/cc @max-wittig @bufferoverflow @fh1ch @rpaik

Edited by Diego Louzán