Keep nil values in arguments

[gdott9]

Commit #7974d48 introduced a change where nil values are rejected from arguments. It can lead to invalid properties with arguments in the incorrect position.

For example, when you add the address in a vCard with something like

vcard.adr postoffice, extended, street, city, region, postalcode, country

the resulting line must have all these fields to respect the corresponding positions defined in the RFC. But if any one of these properties return nil, it will be removed from the arguments array and the following fields will not be in the correct position.

[mcmire]

The reason we introduced this change was because we're seeing vcards being created that have an empty full name field. This is totally possible if you set the full name to nil (because vcardigan will convert it to an empty string). This change indirectly fixed the valid? check that is already in place. We're supposed to be checking that the vcard has a full name. Prior to that commit, this validation had been implemented to check only that the vcard had a full name property.

def validate
  unless @fields['fn']
    raise VCardigan::EncodingError,
      "vCards must include an FN field"
    end
  end
end

Since setting the full name to nil now does nothing, this validation will succeed whether full name has never been set or set to nil. If we revert this then the validation will break. Perhaps we should fix it like so:

def validate
  if !@fields.key?('fn') || @fields['fn'].values.all? { |value| value.to_s.empty? }
    raise VCardigan::EncodingError,
      "vCards must include an FN field"
    end
  end
end

[gdott9]

If I understand correctly, you want to raise VCardigan::EncondingError when the FN field is not defined or if it is defined to nil (with vcard.fn nil).

With my pull request, this behaviour is not modified. When calling vcard.fn nil, the args array will be [nil] and args.any? will still return false so the property will not be added and the error will be raised.

But, we can still set an empty value for the FN field with vcard.fn ''. Your proposition for the validate method could prevent this. A vcard must include a FN field, but I don't know if an empty value is invalid, so is it necessary to add this check?

[mcmire]

Ah I see what this PR is doing then.

According to the vcard spec, the FN property must be present, but there is no mention of what FN must be. I believe Google must be interpreting it to mean that it must be present and non-empty, as we are getting errors on our side when attempting to create vcards. So I think it is necessary to make this check better.

[asherr]

Thanks for the PR @gdott9, as @mcmire explained the purpose of this change was to ensure the validation of FN was functioning properly. You're right that this has unintended consequences like being unable to create a valid address field without all the values. Our primary use-case of vcardigan here at Brewster is syncing to Google via their CardDav API, which is notorious for not matching the RFC specifications. In Google's implementation the FN value is required to be present and not an empty string. I'll merge this pull request and change the validation method to match this expectation. Hopefully that should fix any problems with the ADR field as well.

[mcmire]

Thanks, we've created another PR for this adds your change and then ensures that #valid? works as it should: #10.

[gdott9]

Awesome, thank you for your work on this gem! :+1: