Skip to content
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

Added the option to make Asset serials required #14955

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

Conversation

Godmartinz
Copy link
Collaborator

@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

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.

app/Models/Asset.php Outdated Show resolved Hide resolved
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

app/Providers/ValidationServiceProvider.php Outdated Show resolved Hide resolved
app/Models/Asset.php Outdated Show resolved Hide resolved
@Godmartinz
Copy link
Collaborator Author

Added your suggestions @marcusmoore 👍

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 👍🏾 😄

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.

None yet

2 participants