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 Ldap Company as option and FMCS Notification #14650

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

Conversation

Godmartinz
Copy link
Collaborator

@Godmartinz Godmartinz commented Apr 25, 2024

Description

This adds ldap_company to the settings table as a syncable option for LDAP.

image

This also adds an addition to the notification system, if FMCS is turned on it will notify you in the Navbar with info on the Asset/License/Accessory in question.
image

Notifications are links to the item/s in question.
https://github.com/snipe/snipe-it/assets/47435081/5d10fcba-6f87-4d58-bd62-5ee56d667e49
The notification logic does not fire unless FMCS is turned on.
https://github.com/snipe/snipe-it/assets/47435081/bb5a227a-55ea-48f0-aae2-553830815800

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • [ x] New feature (non-breaking change which adds functionality)

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 Apr 25, 2024

PR Summary

  • Addition of a New LDAP Company Sync Functionality
    The update introduces a new variable ($ldap_result_company) in the LdapSync.php file which allows the application to handle the 'company' field pulled from the LDAP data. This variable plays a key role in enhancing how user data is synchronized from LDAP servers to our application.

  • Ensuring Consistency of New LDAP Field Across Files
    The ldap_company field has been added to SettingsController.php and Setting.php files. This is vital in keeping the application's settings consistent throughout the application and making sure that the new field is recognized and managed properly in the system.

  • Update of Language File for New LDAP Field
    The general.php language file has received an update which now includes the ldap_company field. This change ensures the new field is understood and displayed correctly in the context of the application's localization and internationalization.

  • Integration of New LDAP Field in Templates
    Finally, the ldap_company field has been added to the ldap.blade.php template file. This addition ensures that the new 'company' attribute from LDAP is distributed as per the user interface design, hence improving the user's visual experience.

@Godmartinz Godmartinz marked this pull request as ready for review April 25, 2024 18:59
@Godmartinz Godmartinz requested a review from snipe as a code owner April 25, 2024 18:59
@Godmartinz Godmartinz requested review from uberbrady, snipe and marcusmoore and removed request for snipe April 25, 2024 18:59
@snipe
Copy link
Owner

snipe commented Apr 26, 2024

What happens if the user already had things checked out to them that belong to a certain company, and then the user's company changes with FMCS turned on?

Copy link
Owner

@snipe snipe left a comment

Choose a reason for hiding this comment

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

A few questions we need to think through, I think?

@Godmartinz Godmartinz changed the title Added Ldap Company as a syncable field Added Ldap Company as option and FMCS Notification May 7, 2024
@Godmartinz Godmartinz marked this pull request as draft May 7, 2024 22:46
@Godmartinz Godmartinz marked this pull request as ready for review May 9, 2024 17:30
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