HTML Architect: Cannot open a Data Constant editor

[pdietrich]

Opening the editor of any Data Constant in the HTML Architect returns a server error 500 and the editor never opens.

There are two separate errors, one after the other:

1. The duplicate name check fails - happens for every Data Constant.
2. Loading the drop-down values fails - happens for constants of type Boolean or with a lookup assigned.

Reproducible on current master. Not related to moving items between packages, that is just how I ran into it.

Does anyone else run into this? I think I can fix it quickly, I would estimate the effort as S.

[DanilTkachenko (danilt2000)]

I can try to reproduce it today

[DanilTkachenko (danilt2000)]

Yes, that happens. After a fix, if someone approves it, it is need to add an e2e test for this scenario

[DanilTkachenko (danilt2000)]

Maybe possible fix

NoDuplicateNamesInDataConstantRuleAtribute.cs:54

Change to

public override Exception CheckRule(object instance, string memberName)
{
    if (string.IsNullOrEmpty(memberName))
    {
        CheckRule(instance);
    }

    if (memberName != "Name")
    {
        throw new Exception(
            nameof(NoDuplicateNamesInDataConstantRuleAtribute)
                + " can be only applied to Name properties"
        );
    }

    if (instance is not DataConstant dataConstant)
    {
        return null;
    }

    bool hasDuplicate = dataConstant
        .PersistenceProvider.RetrieveListByCategory<DataConstant>(DataConstant.CategoryConst)
        .Any(constant => constant.Name == dataConstant.Name && constant.Id != dataConstant.Id);
    return hasDuplicate
        ? new DataException(dataConstant.Name + " contains duplicate  names ")
        : null;
}

And EditorPropertyFactory.cs:183

From

return values
    .Cast<ISchemaItem>()
    .Select(x => new DropDownValue(x?.Name ?? "", x?.Id))
    .ToArray();

To

return values
    .OfType<ISchemaItem>()
    .Select(x => new DropDownValue(x?.Name ?? "", x?.Id))
    .ToArray();

[pdietrich]

@DanilTkachenko

Yes, that happens. After a fix, if someone approves it, it is need to add an e2e test for this scenario

Yes, thx for test. Petr assigned it to me for a fix last week - there’s a record of it in Support.

E2E tests are of course an integral part of almost every fix or new addition to ORIGAM, so I’ll add the relevant coverage alongside the fix and open a PR once it’s ready.

[pdietrich]

@DanilTkachenko

Maybe possible fix

Thanks a lot for looking into this, I appreciate the effort.

Up front, so I don’t waste your time: I won’t be taking these patches in. The direction you point at is right, but I’ve already done a fair amount of digging into this area and I’m going to go with my own judgement and research on it.

Your diagnosis of both spots is correct. What the patches don’t cover is that the crash is the visible tip of a larger problem, and a few things there aren’t obvious from the outside:

• A constant’s value is an untyped property, so it has no editor and no parsing of its own. It’s edited as text through its type converter, and that round trip was simply missing on the server side - the crash was just the first thing you hit.
• Values offered for such a property are not always model items. Some are plain values. Making the crash go away by skipping everything that isn’t a model item means the drop down comes up empty, so a loud failure quietly turns into a value the user can’t pick at all. Skipping values isn’t an option here, they have to be carried through.
• That text round trip is culture sensitive. The value leaves the server in one number format and comes back parsed in the format of whatever machine the server runs on, which silently rewrites decimals on a non-English machine. That one is worse than a crash, because nothing tells the user their constant now holds a different number.
• The validation rule has its own set of holes beyond the null one you found, including what it does with an empty name and where its message text belongs.

So the change ends up quite a bit broader than these two spots. Thanks again for the pointers, they were a useful sanity check on the direction.

[pdietrich]

FIX: Data Constant editor