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

Mail authentication can be set as default (works together with LDAP auth) #18807

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

SebSept
Copy link
Contributor

@SebSept SebSept commented Jan 24, 2025

Checklist before requesting a review

Please delete options that are not relevant.

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

Review

It's current a WIP PR.
It does the job but some things may require changes or at least some feedback, for me as glpi new contributor. These are marked with @todo annotations in the code.

By the way I used another way to write test :

  • I did'nt use phpunit assertions to check to tests preparation, I use php native assert() which also trigger failure if needed but do not count as test. I also help to distinguish between real test assertion failures and test preparation failures. I think this is the way we are supposed to write assertion.
  • because these tests are functionnal and not unit test, I used a more appropriate naming which describe the behaviour without naming functions (that good for unit test only).

@SebSept SebSept requested a review from cedric-anne January 24, 2025 13:09
@SebSept SebSept self-assigned this Jan 24, 2025
install/migrations/update_10.0.x_to_11.0.0/authmail.php Outdated Show resolved Hide resolved
*
* @return AuthMail|AuthLDAP|null Auth ID or null if not found
*/
public static function getDefaultAuth(): AuthMail|AuthLDAP|null
Copy link
Member

Choose a reason for hiding this comment

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

This method should probably be moved in the Auth class itself.

src/AuthLDAP.php Outdated
*/
public static function getDefault()
{
// @todo trigger deprecation ?
Copy link
Member

Choose a reason for hiding this comment

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

Here is how to trigger a deprecation message in GLPI.

Suggested change
// @todo trigger deprecation ?
Toolbox::deprecated('The `AuthLDAP::getDefault()` method is deprecated, use `class::method()` instead.');

src/AuthLDAP.php Show resolved Hide resolved
src/AuthLDAP.php Outdated Show resolved Hide resolved
src/AuthLDAP.php Show resolved Hide resolved
src/AuthLDAP.php Outdated Show resolved Hide resolved

if (isset($this->fields['is_default']) && (int) $this->fields["is_default"] === 1) {
// remove from authmails tables
$DB->update(
Copy link
Member

Choose a reason for hiding this comment

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

I know you copied the existing code, but we should avoid using low level methods directly to update an item, as they prevent plugins hooks to be able to handle the corresponding operations. You should replace DBmysql::update() by CommonDBTM::update() here. It requires some refactoring, as you will have first to fetch the existing default server, if any, and the update it.

Same remark for the AuthMail update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:/ It doesn't look like CommonDBTM::update() give the ability to update multiple items with a where criteria.

src/AuthMail.php Outdated Show resolved Hide resolved
@SebSept SebSept force-pushed the add-default-for-authmails branch from c64df6f to 5098c81 Compare January 31, 2025 16:34
@SebSept SebSept requested a review from cedric-anne February 3, 2025 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants