nyaruka/gocommon

Common utility library for the TextIt platform.

gocommon's Issues

Tel Parsing should deal with full tel URNs better

urns.NewTelURNForCountry("tel:[phone]", "US") -> tel:+tel[phone]

Which seems mighty unfortunate considering how many people probably intuitively want to use @urns.tel as the argument to things that expect a telephone number.

Detecting we are given a tel URN seems like it would probably be worth it? What are possible downsides there?

Also feels super weird that the current behavior ends up with + prefixed on the returned number.

@rowanseymour feels liken number parsing this has been under a lot of flux lately, any opinions here?

Cut new release for FC support in Courier/Mailroom

Will need a new release version for nyaruka/courier#247 to pass

+ is a valid phone numbers character

This issue is causing to trim + on number(https://github.com/nyaruka/gocommon/blob/master/urns/urns.go#L289) and return the number missing the + when phonenumbers lib does not consider the number to be valid https://github.com/nyaruka/gocommon/blob/master/urns/urns.go#L305

https://github.com/nyaruka/gocommon/blob/master/urns/urns.go#L70

URN Normalization needs to be idempotent

Normalizing tel[phone] (invalid) returns tel:[phone] (invalid) but normalizing that returns tel:[phone] (valid).

Maybe we need to combine normalization and validation, e.g. NormalizeAndValidate which will only ever return a number which passes phonenumber.parse.

Add more things!

This is more a urns package than a gocommon package.

There are a bunch of things which we could probably move from courier / goflow / mailroom to here to unify our use. Some ideas:

• UUID handling / generation
• fair queueing / task running
• HTTP request / response logging and tracing
• DB utilities for batch inserts / updates (in mailroom)

Use `BCPMatcher` to select locale for date formatting

Bug when using S3 compatible (not amazon) services

I use Minio in my local development environment to support developing on attachments. It doesn't work properly right now, because all the URLs that courier passes to rapidpro are wrong, pointing to Amazon S3 rather than my local minio environment.

gocommon/storage/s3.go

Line 15 in e8b962c

var s3BucketURL = "https://%s.s3.amazonaws.com%s"

is hardcoded to amazon's URL. If it pulled from the config instead, it would be portable to various S3 compatible services.

I could make a PR if you want.

httpx.Trace should include errors in response?

Looking to use Trace in swaggo and it is mostly great but there's a few things that feel like they are missing from a DRY perspective:

• trace works great if we actually reach the HTTP server and get a response, but doesn't do much to help in the case of connection errors etc.. ideally we'd like to have these logged with the trace.
• trace doesn't really have a concept of whether a request was successful or not (ie, was an error) Can easily see the argument that this belongs on the caller's side, but it does feel like checking for 4**/5** status codes (and err above) could automatically qualify a trace as being an "error"?

I guess it is balance between whether we think those two things would cause trouble elsewhere, but I certainly am finding myself wanting to use Trace as a way to say create an HTTPLog as a standalone thing and it isn't so convenient because of the above.

Thoughts @rowanseymour ?

S3 storage urls should include the bucket region

The URL returned for stored files does not include the bucket region.

Attachments are stored in the correct bucket but the URL returned and stored on the message is incorrect.
The URL is defined here https://github.com/nyaruka/gocommon/blob/main/storage/s3.go#L19

As per the S3 naming conventions the bucket region should form part of the endpoint URL
https://docs.aws.amazon.com/general/latest/gr/s3.html

BulkQuery should support more complex `VALUES` clauses

dbutil.BulkQuery currently only works when statement includes the exact text VALUES( but sometimes depending on formatting style it might be preferable to write it VALUES (.

Would be useful if it allowed casts as well, e.g. VALUES (:id::int) so that the column is created with type int and you don't have to use ::int elsewhere in the query.