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

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

Closed
mxr576 opened this issue Aug 21, 2023 · 10 comments · Fixed by #919
Closed
Labels
enhancement New feature or request

Comments

@mxr576
Copy link
Contributor

mxr576 commented Aug 21, 2023

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

@mxr576 mxr576 added the enhancement New feature or request label Aug 21, 2023
@FCsongradi
Copy link
Contributor

I've started to work on the proposed solution.

@shishir-intelli
Copy link
Collaborator

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

@shishir-intelli
Copy link
Collaborator

Re-opening for 2.x backport of #919..

@shishir-intelli
Copy link
Collaborator

@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
Copy link
Contributor Author

mxr576 commented Sep 6, 2023

@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: #927

@shishir-intelli
Copy link
Collaborator

@mxr576 ,
Thanks for #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
Copy link
Collaborator

@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
Copy link
Contributor Author

mxr576 commented Sep 7, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

shishir-intelli commented 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
None yet
Development

Successfully merging a pull request may close this issue.

3 participants