apigee / apigee-edge-drupal

The Apigee Edge module enables you to integrate a Drupal 9 or 8 site with Apigee.
https://www.drupal.org/project/apigee_edge
GNU General Public License v2.0
32 stars 45 forks source link

Provide a programming API for opting out/fine tuning developer removal on user removal #915

Closed mxr576 closed 1 year ago

mxr576 commented 1 year ago

Is your feature request related to a problem?

The Apigee Edge module nicely integrates with the user cancellation API in Drupal and it follows the development workflow that the API enforces on Drupal developers, more specifically, it implements hook_user_delete() for handling user removal instead of reacting on user_cancel_delete cancel method.

This API provides an ideal approach for canceling/removing user account on the Drupal HTML UI, however, user removal can happen programmatically and when that happens Drupal developers may not always want to remove developer accounts from Apigee as well (and that destroy their API keys). (When the Apigee Monetization feature is enabled, a developer cannot be removed in given circumstances, the Monetization API does not allow that.)

The problem with this approach (again enforced by the user cancellation API) that without additional tweaks there is no way to opt out or tweak how the developer removal should happen when a user is deleted. Should it be executed synchronously (ideal for removing one user) or asynchronously in batch (ideal for bulk removal)?

Describe the solution you would like

Extract the current implementation of apigee_edge_user_delete() to a new service and call that service inside the method. Just simply introducing a service (class+interface) should give enough ammunition for downstream Drupal developers via the service decorator pattern to tweak if and how the developer removal happens when a user is deleted.

Describe alternatives you have considered

Additional context

FCsongradi commented 1 year ago

I've started to work on the proposed solution.

shishir-intelli commented 1 year ago

@mxr576 and @FCsongradi Thank you for your contribution, We will review the PR.

shishir-intelli commented 1 year ago

Re-opening for 2.x backport of https://github.com/apigee/apigee-edge-drupal/pull/919..

shishir-intelli commented 1 year ago

@mxr576 and @FCsongradi , 2.x branch Drupal-check failing for PHP 8.0 after merging 2.x PR,

Error - Syntax error, unexpected T_STRING, expecting T_VARIABLE on line 37 See https://github.com/apigee/apigee-edge-drupal/actions/runs/6069673235

mxr576 commented 1 year ago

@shishir-intelli Sorry, I have just seen this ping. I was surprised to see that the PR could have been and has been merged with this PHP 8.0 compatibility issue. Shouldn't the configuration on GH block the merge when a CI task is failing?

Follow up: https://github.com/apigee/apigee-edge-drupal/pull/927

shishir-intelli commented 1 year ago

@mxr576 , Thanks for https://github.com/apigee/apigee-edge-drupal/pull/927, The Github action is not testing the PR files for PHPCS or Drupal check, because of that, the test gets through without any errors as it test the files from the main branch(3x/2x).

Post merge your PR it tested the latest files and showed Drupal-check error. My advice would be to configure your GH Action so it would be easier to know about the errors for PR's.

shishir-intelli commented 1 year ago

@mxr576 The #927 failed again, please check https://github.com/shishir-intelli/apigee-edge-drupal/actions/runs/6094077960/job/16534890775

IMO its causing because of private readonly LoggerInterface $logger at line number 51

mxr576 commented 1 year ago

Okay, I am lost while that PR is keep failing and do not have time at the moment to spin up a PHP 8.0 env....

shishir-intelli commented 1 year ago

No, don't look at this https://github.com/apigee/apigee-edge-drupal/actions/runs/6101452459, this will fail everytime, as i said earlier that the test are running on main branch(2x) and not with latest PR files, I will run this test on my fork and check. I think it should pass now.

shishir-intelli commented 1 year ago

Test running here https://github.com/apigee/apigee-edge-drupal/actions/runs/6106726471/job/16572330793