jsgoupil / quickbooks-sync

Sync Quickbooks Desktop
MIT License
89 stars 40 forks source link

OwnerID doesn't support "0" value for custom fields #26

Closed tofer closed 5 years ago

tofer commented 5 years ago

Multiple request, response, and ret types use property OwnerID, and are of type GUIDTYPE or GUIDTYPE[], but this does not support the allowed value of "0" to retrieve or set custom fields from QuickBooks. Limiting to GUIDs seems to limit compatibility with private data only.

Note: I did try new GUIDTYPE("0") but that gets formatted to {00000000-0000-0000-0000-000000000000}

I found the following quickbooks documentation in chapter 11 (page 137) for more info on the difference between custom fields and private data, and page 146 for an example query with the "0" value: https://developer-static.intuit.com/qbSDK-current/doc/PDF/QBSDK_ProGuide.pdf

I did some very limited testing, and changing OwnerID to string or string[] I was able to get custom fields successfully via the DataExtRet.

jsgoupil commented 5 years ago

That is the magic code. The OwnerID of zero is reserved for custom fields. If you wanted to make it private data, you would use a GUID here, like this:

Yay for magic! Everybody loves magic. The types are controlled by the XML definition. Obviously, intuit likes to not follow it. I have changed it in the past, that's where we should start. I'll see how easy this one is.

tofer commented 5 years ago

Leave it to Intuit, lol.

Would it be possible to add custom serialization to those properties instead, so it can serialize Guid.Empty to "0" instead of {00000000-0000-0000-0000-000000000000}?

tofer commented 5 years ago

OK, I did another hack that didn't require changing the property type on the XSD generated types. I only did simple testing again, but your unit tests do pass, by modifying the GUIDTYPE class with some "magic zero" handling:

public partial class GUIDTYPE : ITypeWrapper, IComparable<GUIDTYPE>, IXmlSerializable
{
    protected Guid value;

    // Support for Intuit's "magic code" for OwnerID of "0"
    private bool _magicZero;

    public GUIDTYPE()
    {
        this.value = Guid.Empty;
    }

    public GUIDTYPE(string value)
    {
        this.value = Parse(value);
        if (value == "0") _magicZero = true;
    }

    public GUIDTYPE(Guid value)
    {
        this.value = value;
    }

    public override string ToString()
    {
        if (_magicZero && value == Guid.Empty)
        {
            return "0";
        }

        return value.ToString("B", CultureInfo.InvariantCulture);
    }

    public Guid ToGuid()
    {
        return value;
    }

    //...

    public void ReadXml(System.Xml.XmlReader reader)
    {
        reader.MoveToContent();
        var isEmptyElement = reader.IsEmptyElement;
        reader.ReadStartElement();
        if (!isEmptyElement)
        {
            var str = reader.ReadContentAsString();
            _magicZero = str == "0";
            value = Parse(str);

            reader.ReadEndElement();
        }
    }

    //...
}

With simple usage request.OwnerID = new [] { new GUIDTYPE("0") };

In theory, this would then still allow the same functionality in all other purposes, even still allowing {00000000-0000-0000-0000-000000000000} to be parsed without then outputting "0", but I'm obviously not as familiar with the rest of your library to know what ramifications this might have.

jsgoupil commented 5 years ago

I think that's a good alternative, do you think you can submit a pull request with some unit tests?

tofer commented 5 years ago

This could arguably be simpler by always returning 0 for Guid.Empty. Do you have any insight on if that would cause issues for other uses of GUIDTYPE?

jsgoupil commented 5 years ago

I looked at all the usage of OwnerId and ... it's hard to tell how it should behave. Their documentation here: https://developer-static.intuit.com/qbSDK-current/Common/newOSR/index.html says

A GUIDTYPE value can be zero (0), or it can take the form {XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX} where X is a hexadecimal digit. For example: {6B063959-81B0-4622-85D6-F548C8CCB517}

Now technically, 00000000-0000-0000-0000-000000000000 is a valid guid. So I think we should allow it. Especially if people started to use it, then we would break them. So your magicZero idea code is a good idea.

tofer commented 5 years ago

OK sounds good. I'll write up some tests and submit a pull request. Thanks for the fast responses.

jsgoupil commented 5 years ago

Make sure to not drop curling braces to keep consistency :)

tofer commented 5 years ago

Pull request #27 made

jsgoupil commented 5 years ago

Merged. Thanks!