Fix migration of split by scheme rulesets

[rowanseymour]

Should use @(urn_parts(urns.<scheme>).path) for all schemes

[nicpottier]

What are we doing now? This all sounds very familiar now, where the hell were we all talking about this, can't find the ticket now..

[rowanseymour]

tel_e164 becomes @(urn_parts(urns.tel).path), other schemes become @(format_urn(urns.<scheme>))

See https://github.com/nyaruka/floweditor/issues/676

[nicpottier]

There it is, was searching for channel related issues, doh.

So in the anon case, .path is ****** is that right? What is format doing in those?

[rowanseymour]

Redacted URNs evaluate to ********, and:

urn_parts("********") => {scheme: "", path: "********", display: ""}
format_urn("********") => ERROR

not so worried about behavior of format_urn here since if we switch these to @(urn_parts(urns.<scheme>).path) then I don't think anon orgs have any reason to be using format_urn

[rowanseymour]

Tho does seem for simplicity that format_urn(nil) should return "" so you don't have to know if a URN type is set

[nicpottier]

Above do you mean that redacted URNs evaluate to facebook:*******? Not sure I understand how urn_parts stuff works otherwise.

Feels to me like error on format_urn(nil) is right, what's the scenario where that gets us into trouble?

[rowanseymour]

Couldn't remember for sure so added some tests https://github.com/nyaruka/goflow/blob/master/flows/engine/session_test.go#L48 and it's ******** but I'm honestly wondering if I broke that at some point cuz I do vaguely recall us talking about the need for anon orgs to access URN schemes.

facebook:******* seems righter...

urn_parts works cuz you can throw any string at it and it just makes it the path

[nicpottier]

May be running into validation issues there some **** is not always a valid path but in a way that's the point.

Does seem like it should be facebook:**** though for sure. Knowing scheme of say @contact.urn seems pretty needed.

Also feels to me like urn_parts should be blowing up for things that aren't URNs. (not going as far as validating but at least scheme:path format.

[rowanseymour]

Don't disagree about urn_parts but bear in mind that's just calling URN.ToParts which can't error, and if that can error then so can URN.Normalize and URN.Localize etc.

And if we change urn_parts to error... then these router cases all error... whereas currently urn_parts("").path => "".. gives us a way to get a URN path without knowing if it exists.

Re redacting URNs tho I think we agree that's currently wrong and should actually behave like:

@contact.urn => tel:********
@(urn_parts("tel:********")) => {scheme: tel, path: ********}
@(format_urn("tel:********")) => ********

Right?

[nicpottier]

Yes, I think that's right. I do think erroring on something that isn't a URN is right, though it can be an invalid URN.

So just for my own edification, how was the old migration causing those splits not to work as before?

[rowanseymour]

Is this some reverse psychology ? 😁 I thought I was the stickler for syntactical purity and you were pragmatist who'd say no to something that will error unless wrapped in a default.. and then you'd call that shit work.

I'm down with urn_parts and format_urn both blowing up for non URNs.. I just don't like that that it means this flow would generate a bunch of errors when the user is just trying to check URN existence. So does that mean the operand for scheme checks should be default(urn_parts(urns.<scheme>).path, "") ?

I was assuming it was because format_urn was erroring on the redacted URNs and then has_text just sees an error and the case can't match... I think that matches what we're seeing...

Need https://github.com/nyaruka/gocommon/pull/24 btw. We can leave URN.ToParts as is with the assumption that a URN instance is always structurally valid. Then use urns.Parse to create URNs from strings instead of just casting.

[nicpottier]

I think we should probably verify why exactly the current stuff isn't working before going any farther or we may be fixing the wrong thing.

Isn't a split by scheme just @(urn_parts(contact.urn).path))? Or at least that seems like the most common one where you then have a rule per scheme then. But ya, if you were checking if a contact HAD a whatsapp scheme, not just that whatsapp was their primary URN then you would need the default. I think I'm ok with that.

At least for this case I'm not particularly bothered by an expression error. To me that's a lot more useful than an empty result, which could be any number of things. No idea if that is inconsistent with previous thinking, I stand on my current ground though!

[rowanseymour]

The flow we're trying to fix isn't splitting by scheme - it's a series of rulesets splitting on specific schemes https://app.rapidpro.io/flow/editor/32d652c2-0404-4c0a-828c-e627e33fcaeb/

And the problem is that we're currently migrating @contact.facebook to @(format_urn(urns.facebook)). For an anon org, that currently errors, so never matches even if the contact has a Facebook URN. That doesn't match what the new editor is doing for new flows (it uses @(urn_parts(urns.facebook).path)))

I would argue that expression errors usually indicate to the user that there's something to fix. Just adding a Split By Field and picking some URN types because you want to see what URN types the contact has shouldn't dump out a bunch of expression errors... it's going to be more misleading than useful because they can't even see or edit these expressions.

So I think we need default() to stop these from erroring when that URN type isn't set

[nicpottier]

I would argue that expression errors usually indicate to the user that there's something to fix. Just adding a Split By Field and picking some URN types because you want to see what URN types the contact has shouldn't dump out a bunch of expression errors... it's going to be more misleading than useful because they can't even see or edit these expressions.

Oh oh, yes you are right. Sorry for some reason I thought we were doing more of a "Split by Channel Type" which was then using the more generic (and less likely to error) @(urn_parts(contact.urn).scheme).

Totally agree that we shouldn't be throwing errors for expressions we generate, so agree with the default change.

Do wonder if we should just add a "Split by Scheme" that makes this easier for users, seems like doing a single split as opposed to lots of cascading ones is a lot nicer.

[rowanseymour]

Yeah split by scheme sounds useful - I imagine it's common to support a few different scheme types but not multiple schemes per contact.. which is more complicated to gather.

Anyways you already had that idea: https://github.com/nyaruka/floweditor/issues/630

[rowanseymour]

Fixed in #745