WIP: Improve postgresql support

[landryb]

Description

WIP to Fixes #13615 & #7600

the rationale is to:

• validate fields as booleans when they're checkboxes
• provide proper defaults when reading values from the POST'ed settings

with those two commits i can save settings on general & branding pages, will check for others....

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tested on postgresql only, needs testing on mysql. php 8.1, using 6.4.2, on OpenBSD w/ nginx.

Checklist:

• I have read the Contributing documentation available here: https://snipe-it.readme.io/docs/contributing-overview
• I have formatted this PR according to the project guidelines: https://snipe-it.readme.io/docs/contributing-overview#pull-request-guidelines
• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[welcome]

💖 Thanks for this pull request! 💖

We use semantic commit messages to streamline the release process and easily generate changelogs between versions. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix if it doesn't have one already.

Examples of commit messages with semantic prefixes:

• Fixed #<issue number>: don't overwrite prevent_default if default wasn't prevented
• Added #<issue number>: add checkout functionality to assets
• Improved Asset Checkout: use new notification method for checkout

Things that will help get your PR across the finish line:

• Document any user-facing changes you've made.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[what-the-diff]

PR Summary

• Default Value Assignments in SettingsController.php
  The system now ensures that if no value is provided by the user, default values are assigned to five settings - require_accept_signature, show_assigned_assets, allow_user_skin, show_url_in_emails, and logo_print_assets. All of these will be set to '0'.

• Validation Rule Updates in Setting.php
  The validation checks for the settings login_remote_user_enabled and login_common_disabled have been updated to accommodate boolean (true/false) values. This allows for more efficient error-checking and data validation.

[landryb]

saving settings on security page fails:

SQLSTATE[23502]: Not null violation: 7 ERROR:  null value in column "login_remote_user_custom_logout_url" of relation "settings" violates not-null constraint

tried the same trick in postSecurity but for some reason it doesn't avoid the exception.

-            $setting->login_remote_user_custom_logout_url = $request->input('login_remote_user_custom_logout_url');
+            $setting->login_remote_user_custom_logout_url = $request->input('login_remote_user_custom_logout_url', '');

[snipe]

Thanks for looking into this - can you please make these changes against the develop branch though? As we're preparing for v7's launch, develop is the v7 codebase.

[snipe]

(Unfortunately, you probably won't be able to retarget cleanly)

[landryb]

ah sorry, didn't realize master was the stable branch, will rebase/cherrypick the commits (note that i've only tested on 6.4.2, and given there's a laravel update in develop it might need extra testing with postgresql & v7..)

[landryb]

i've checked and i can successfully save settings on:

• localization
• barcodes
• ldap (including testing a working ldap bind to openldap)
• asset tags
• notifications

saving settings on label page fails:

SQLSTATE[23502]: Not null violation: 7 ERROR:  null value in column "label2_enable" of relation "settings" violates not-null constraint
SQLSTATE[23502]: Not null violation: 7 ERROR:  null value in column "label2_asset_logo" of relation "settings" violates not-null constraint

i've tried the following but it's not enough, i still get the null violation on label2_asset_logo, which in the psql db is a boolean type defaulting to false:

-        $setting->label2_enable = $request->input('label2_enable');
+        $setting->label2_enable = $request->input('label2_enable', '0');
         $setting->label2_template = $request->input('label2_template');
         $setting->label2_title = $request->input('label2_title');
-        $setting->label2_asset_logo = $request->input('label2_asset_logo');
+        $setting->label2_asset_logo = $request->input('label2_asset_logo', '0');

[snipe]

Weird that that's not working there... that would be the Laravel way to set a default for those values. 🤔

[landryb]

another thing that i noted, since i've the allow_user_skin fix applied, the skin is broken and i have this in the log (unformated)

[2024-05-30 14:54:58] production.ERROR: Unable to locate Mix file: /css/dist/skins/skin-blue                                                                                          
                                                                                                 .css. {"userId":2,"exception":"[object] (Exception(code: 0): Unable to locate Mix file: /css/dist/skins/skin-blue                                                                                                                                                          
                                 .css. at /snipeit/vendor/laravel/framework/src/Illuminate/Foundation/Mix.php:61)                                                                     

as if https://github.com/snipe/snipe-it/blob/develop/resources/views/layouts/default.blade.php#L39 didn't contain '' anymore?

if i look at the generated HTML there's plenty of spaces between skin-blue (the default) and .css.

snipeit=> select length(skin) from settings;
 length 
--------
      4
(1 row)

snipeit=> select substr(skin,0,5) from settings;
 substr 
--------
 blue
(1 row)

the skin field type is character(191), and i dont see the value padded with spaces... puzzing :)

GitHub

snipe-it/resources/views/layouts/default.blade.php at develop · snipe/snipe-it

A free open source IT asset/license management system - snipe/snipe-it

[landryb]

--- a/resources/views/layouts/default.blade.php
+++ b/resources/views/layouts/default.blade.php
@@ -36,7 +36,7 @@ dir="{{ in_array(app()->getLocale(),['ar-SA','fa-IR', 'he-IL']) ? 'rtl' : 'ltr'
         <link rel="stylesheet" href="{{ url(mix('css/dist/skins/skin-'.Auth::user()->present()->skin.'.min.css')) }}">
     @else
         <link rel="stylesheet"
-              href="{{ url(mix('css/dist/skins/skin-'.($snipeSettings->skin!='' ? $snipeSettings->skin : 'blue').'.css')) }}">
+              href="{{ url(mix('css/dist/skins/skin-'.($snipeSettings->skin!='' ? trim($snipeSettings->skin) : 'blue').'.css')) }}">
     @endif
     {{-- page level css --}}
     @stack('css')

that restores a working skin but feels... meh?

[landryb]

Another buglet passing by.. sometimes visiting the asset list for a given order (eg hardware?order_number=DV10130) triggers this

[previous exception] [object] (PDOException(code: 42703): SQLSTATE[42703]: Undefined column: 7 ERROR:  column \" \" does not exist                                                    
LINE 1: ...text LIKE $32) or (CONCAT(assets_users.first_name,\" \",assets...                                                                                                          
                                                             ^ at /snipeit/vendor/laravel/framework/src/Illuminate/Database/Connection.php:373)   

and the list of assets shown is empty

[snipe]

I'm not sure that's related though.

If you run the following query, do you get any results?

select id, asset_tag, assigned_to, created_at, updated_at from assets where assigned_to IS NOT NULL and assigned_type IS NULL AND deleted_at is NULL

[snipe]

(It might be related - I'm not sure.)

[landryb]

hm sorry, you're right that's unrelated, reloading the hardware by order id page doesn't trigger the traceback i saw in the log (which is probably coming from visiting another page). It's just weird that the list is empty, given i get there by clicking on the order id associated to an asset. I was pretty sure that feature worked/works... i've seen it working at some point, at least. I'll dig further, and also try to figure out what page triggered the sql exception i saw

[snipe]

When we see that, it's usually because some data got into the database that's a little off. The query I gave you above should find any assets that are checked out to a user (assigned_to has a value) but assigned_type isn't populated. That shouldn't normally happen, but if we missed validation somewhere, it's possible.

[landryb]

When we see that, it's usually because some data got into the database that's a little off. The query I gave you above should find any assets that are checked out to a user (assigned_to has a value) but assigned_type isn't populated. That shouldn't normally happen, but if we missed validation somewhere, it's possible.

i have nothing returned by this request, eg all assigned assets are either assigned properly to a location or a user

what i found strange in the sql exception was the Undefined column: 7 ERROR: column \" \" does not exist in LINE 1: ...text LIKE $32) or (CONCAT(assets_users.first_name,\" \",assets... - will try to figure out from the traceback where the framework generates this broken SQL

[snipe]

We're also seeing internally if there isn't an easy way to set up an automated pgSQL test for GH actions. I expect some tests would fail normally anyway (we have to skip some tests in sqlite, for example because the test framework doesn't handle it exactly right) but we could probably try to add something for pgSQL as well, at least to give us a heads up, once we get this to a more-working position.

[snipe]

(Stupidly, speaking of GH actions, Codacy for some reason is saying this PR created 2 new issues, but when I go to the dashboard on Codacy, it can't seem to find any new issues from the PR. Sigh. Technology, man.)

[snipe]

another thing that i noted, since i've the allow_user_skin fix applied, the skin is broken and i have this in the log (unformated)

That's not even a pgSQL issue though, that's Laravel mix.

Shot in the dark, but if you try

<link rel="stylesheet" href="{{ url(mix('css/dist/skins/skin-'.($snipeSettings->skin ?? 'blue').'.css')) }}">

using the null coalescence operator instead, does that work any better? (We're going to be moving a lot of stuff over to that anyway in v7.)

that restores a working skin but feels... meh?

It does feel meh, but I suppose it won't hurt anything to keep it. Just not sure why we'd need it - there isn't any space padding in the DB for those...

[landryb]

Shot in the dark, but if you try

<link rel="stylesheet" href="{{ url(mix('css/dist/skins/skin-'.($snipeSettings->skin ?? 'blue').'.css')) }}">

using the null coalescence operator instead, does that work any better? (We're going to be moving a lot of stuff over to that anyway in v7.)

nah, with that sadly the skin is still broken (eg $snipeSettings->skin is replaced by blue <manyspaces> and the css is not found)

edit: it works if i use trim and the null coalescence operator:

-              href="{{ url(mix('css/dist/skins/skin-'.($snipeSettings->skin!='' ? $snipeSettings->skin : 'blue').'.css')) }}">
+              href="{{ url(mix('css/dist/skins/skin-'.(trim($snipeSettings->skin) ?? 'blue').'.css')) }}">

i can also work around this issue by nulling the skin field in the db :)