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.

Sign up for GitHub

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

Jump to bottom

target_suitecrm: add functionality to disable accounts instead of delete #108

Merged
merged 4 commits into from
Dec 8, 2023
Merged

target_suitecrm: add functionality to disable accounts instead of delete #108

merged 4 commits into from
Dec 8, 2023

Conversation

thinkl33t
Copy link
Contributor

Currently we delete any accounts which no longer exist in ldap. We suspect this will cause historic reporting to fail, and the recommendation from suitecrm is to disable users instead.

@thinkl33t
Copy link
Contributor Author

thinkl33t commented Dec 8, 2023 •
edited
Loading

edit: now resolved

This currently decreases the test coverage. Need to add tests for:

  • making sure a user is not deleted if they're on the excluded_users list
  • makeing sure a user is disabled if delete_absent_users is false
  • making sure a user is not disabled if they're on the excluded_users list and delete_absent_users is false

@thinkl33t
target_suitecrm: add functionality to disable accounts instead of delete
1613fd1 
Currently we delete any accounts which no longer exist in ldap.  We 
suspect this will cause historic reporting to fail, and the 
recommendation from suitecrm is to disable users instead.
@thinkl33t thinkl33t force-pushed the bobclough/suitecrm_disable_users branch from 9a9a54a to 1613fd1 Compare December 8, 2023 12:32
@thinkl33t thinkl33t force-pushed the bobclough/suitecrm_disable_users branch from fdec633 to 13bd2a3 Compare December 8, 2023 15:07
richardmaw-codethink
richardmaw-codethink approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@richardmaw-codethink richardmaw-codethink left a comment

Choose a reason for hiding this comment

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

It looks correct, though there's some stylistic changes that might be an improvement.

tests/test_target_suitecrm.py Outdated Show resolved Hide resolved
tests/test_target_suitecrm.py Outdated Show resolved Hide resolved
lifecycle/target_suitecrm.py Show resolved Hide resolved
@thinkl33t thinkl33t force-pushed the bobclough/suitecrm_disable_users branch from 51f7b81 to 328b590 Compare December 8, 2023 16:57
@thinkl33t thinkl33t merged commit e8fae3c into main Dec 8, 2023
8 checks passed
@thinkl33t thinkl33t deleted the bobclough/suitecrm_disable_users branch December 8, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyianperkins garyianperkins garyianperkins approved these changes

@richardmaw-codethink richardmaw-codethink richardmaw-codethink approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thinkl33t @richardmaw-codethink @garyianperkins