Skip to content

Added the option to make Asset serials required and error messages #14955

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

Godmartinz
Copy link
Member

@Godmartinz Godmartinz commented Jun 24, 2024

Description

So this adds a validation rule require_serial. This makes a serial required or nullable based on a boolean in the settings:

image adds an error and message for required and unique options being selected if there are serials that are not filled or unique.

Unique error:
image
Required error:
image

Fixes #FD-41625

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Test Configuration:

  • PHP version:
  • MySQL version
  • Webserver version
  • OS version

Checklist:

Copy link

what-the-diff bot commented Jun 24, 2024

PR Summary

  • Addition of Required Serial to Settings
    The SettingsController now includes a parameter $setting->required_serial to its postSettings method. This helps in identifying whether a serial is needed in the settings.

  • Asset Modifications to Incorporate Required Serial
    A parameter $required_serial has been added to the Asset object by way of the __construct method. The impact of this is a new rule can be added for identifying serial based on the value of $required_serial.

  • Database Migration for Required Serial
    There's a new migration file 2024_06_24_173857_adds_serial_required_to_settings_table.php introduced. This file is useful as it adds a required_serial column to the settings table, extending its functionality related to serial requirements.

  • Language File Expansion for Required Serial
    The language file general.php is now tuned to accommodate new translation keys namely required_serial and require_serial_help_text. End-users will see this as additional localization supports for the required serial configuration.

  • Incorporating Required Serial in the View
    The view file general.blade.php has been adapted to include a new form input for the required_serial setting with corresponding label and help text. This enhances the user interface by factoring in a new possible user input for serial requirements.

@Godmartinz Godmartinz changed the title Added the option to make serials required Added the option to make Asset serials required Jun 24, 2024
Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there might be a better implementation. Let me know what you think.

@Godmartinz Godmartinz requested a review from marcusmoore June 24, 2024 23:42
Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a couple comments that don't hold up merging.

One additional thing (that doesn't hold up the PR) is that it would be cool if we had the field do native html validation by adding required to the input tag like we do with Asset Tag:

native html validation

@Godmartinz
Copy link
Member Author

Added your suggestions @marcusmoore 👍

@Godmartinz Godmartinz requested a review from marcusmoore June 26, 2024 00:06
Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍🏾 😄

@@ -242,7 +242,8 @@ public function edit($assetId = null)

return view('hardware/edit', compact('item'))
->with('statuslabel_list', Helper::statusLabelList())
->with('statuslabel_types', Helper::statusTypeList());
->with('statuslabel_types', Helper::statusTypeList())
->with('settings', Setting::getSettings());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snipe
Copy link
Member

snipe commented Jul 8, 2024

This looks good - might we want to warn folks here if their existing assets do not meet the required and/or uniqueness constraints?

@Godmartinz
Copy link
Member Author

This looks good - might we want to warn folks here if their existing assets do not meet the required and/or uniqueness constraints?

image Have a response for required. I'll add one for uniqueness as well 👍

@Godmartinz Godmartinz changed the title Added the option to make Asset serials required Added the option to make Asset serials required and error messages Jul 8, 2024
@Godmartinz Godmartinz requested a review from snipe July 8, 2024 17:40
@Godmartinz Godmartinz requested a review from marcusmoore July 8, 2024 17:40
@snipe
Copy link
Member

snipe commented Aug 22, 2024

@Godmartinz sorry, looks like we have some more conflicts here

@Godmartinz
Copy link
Member Author

Conflicts resolved @snipe 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants