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

Extract user deletion into a new service #918

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

FCsongradi
Copy link
Contributor

@FCsongradi FCsongradi commented Aug 25, 2023

Pull request for issue 915.

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Merging #918 (35e672b) into 2.x (ae0e997) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##                2.x     #918   +/-   ##
=========================================
  Coverage     43.90%   43.90%           
  Complexity     3030     3030           
=========================================
  Files           343      343           
  Lines         11136    11136           
=========================================
  Hits           4889     4889           
  Misses         6247     6247           

src/UserRemovalHandler.php Outdated Show resolved Hide resolved
src/UserRemovalHandler.php Outdated Show resolved Hide resolved
src/UserRemovalHandler.php Outdated Show resolved Hide resolved
src/UserRemovalHandler.php Outdated Show resolved Hide resolved
$developer = $this->entityTypeManager->getStorage('developer')->load($account->getEmail());
// Sanity check, the developer may not exist in Apigee Edge.
if ($developer) {
$developer->delete();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to have an info level log message in logs about the removal of the developer. I know this is a change compared with the original implementation, but only a minor one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added info level logging when the developer is deleted.

src/UserRemovalHandlerInterface.php Outdated Show resolved Hide resolved
apigee_edge.module Outdated Show resolved Hide resolved
@@ -38,6 +38,7 @@ use Drupal\apigee_edge\Form\DeveloperSettingsForm;
use Drupal\apigee_edge\JobExecutor;
use Drupal\apigee_edge\Plugin\Field\FieldType\ApigeeEdgeDeveloperIdFieldItem;
use Drupal\apigee_edge\Plugin\Validation\Constraint\DeveloperEmailUniqueValidator;
use Drupal\apigee_edge\UserRemovalHandler;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe place this class and the interface under the Drupal\apigee_edge\User namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored

@mxr576
Copy link
Contributor

mxr576 commented Aug 28, 2023

This is a 2.x backport of #919.

@FCsongradi FCsongradi marked this pull request as ready for review August 28, 2023 09:33
@mxr576
Copy link
Contributor

mxr576 commented Aug 30, 2023

@FCsongradi #919 has changes, backporting them can wait until a review from maintainers.

* Extract user deletion into a new service

Introduce a programming API for downstream developers that allows
customizing what should happen when a user is deleted in Drupal.

Co-authored-by: Dezső BICZÓ <[email protected]>

* Leverage the new user removal handler API for cascade deleting team roles

* Better naming for the new API and implementations

---------

Co-authored-by: ferenc.csongradi <[email protected]>
@mxr576
Copy link
Contributor

mxr576 commented Sep 1, 2023

The build failed because Codecove update failed.

Should be ready for review.

@shishir-intelli
Copy link
Collaborator

The build failed because Codecove update failed.

Should be ready for review.

@mxr576
Lets rerun it, probably codecov uploading issue.

Copy link
Collaborator

@shishir-intelli shishir-intelli left a comment

Choose a reason for hiding this comment

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

LGTM

@shishir-intelli shishir-intelli added this to the 2.1.2 milestone Sep 4, 2023
@shishir-intelli shishir-intelli added the enhancement New feature or request label Sep 4, 2023
@shishir-intelli shishir-intelli merged commit cdcf64a into apigee:2.x Sep 4, 2023
5 checks passed
mxr576 added a commit to FCsongradi/apigee-edge-drupal that referenced this pull request Sep 6, 2023
mxr576 added a commit to FCsongradi/apigee-edge-drupal that referenced this pull request Sep 6, 2023
mxr576 added a commit to FCsongradi/apigee-edge-drupal that referenced this pull request Sep 6, 2023
shishir-intelli pushed a commit that referenced this pull request Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

3 participants