Set user agent columns to `null` when user agent is not available

[stevebauman]

Closes #243

This PR implements a fix to properly set the user agent columns to null instead of "0" when they are not available.

I've also corrected the ShortURLVisit model annotations to present these attributes as nullable.

Let me know if you'd like anything adjusted. Thanks for your time!

[ash-jc-allen]

Hey @stevebauman, thanks for the PR! Good spot on this one. I think this was a silly oversight on my behalf when I first added the tracking functionality 🤦‍♂️

I like the idea behind these changes, but I'm wondering what effect it might have on existing apps. For example, some devs might have already built their apps (and queries) around expecting that the values might be false if the values couldn't be determined when recording. So this means their short_url_visits table would potentially include false and null values.

Do you have any thoughts on this? 🙂

[stevebauman]

Yea that's definitely a possibility. We could add a new migration to migrate these values to null?

public function up()
{
    DB::table('short_url_visits')->update([
        'ip_address' => DB::raw("CASE WHEN ip_address = '0' THEN NULL ELSE ip_address END"),
        'operating_system' => DB::raw("CASE WHEN operating_system = '0' THEN NULL ELSE operating_system END"),
        'operating_system_version' => DB::raw("CASE WHEN operating_system_version = '0' THEN NULL ELSE operating_system_version END"),
        'browser' => DB::raw("CASE WHEN browser = '0' THEN NULL ELSE browser END"),
        'browser_version' => DB::raw("CASE WHEN browser_version = '0' THEN NULL ELSE browser_version END"),
        'referer_url' => DB::raw("CASE WHEN referer_url = '0' THEN NULL ELSE referer_url END"),
        'device_type' => DB::raw("CASE WHEN device_type = '0' THEN NULL ELSE device_type END"),
    ]);
}

It's up to you of course whether you consider this a bug fix, or a breaking change.

[ash-jc-allen]

Hey @stevebauman! Sorry for such a big delay with getting this looked at properly. Personal life has been manic recently. But I've decided I'm gonna take half of what you've added here haha.

I'm gonna add a thin abstraction layer over the top of the user agent detection because I want to swap out jenssegers/agent for another package. This is to avoid any Composer conflicts (which we're having at the moment with Jetstream projects). But I'll make sure in this new layer that we're defaulting to null if the data can't be detected properly 😄

But I think I'll update this PR at some point to use the new migration you've mentioned above, because I do appreciate the effort you put in to this!

[ash-jc-allen]

Hey @stevebauman! I'm just dropping a quick message to say that I've implemented the thin abstraction layer. So I'm going to close this PR.

But I definitely think your proposed database migration is useful though. So I've added it in https://github.com/ash-jc-allen/short-url/pull/272.

I'll make sure you get a mention in the changelog for this because it was 100% your idea! 😄

[stevebauman]

Hey @ash-jc-allen! Oh no worries at all man, no need to apologize!

Also no need for any of that unless you've already done it -- I do appreciate it though! However you want to do things is totally fine with me.

Thanks for your continued work on this! 🙏