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

Handle graphql consumers for both BNF and Go #1891

Conversation

spaceo
Copy link
Collaborator

@spaceo spaceo commented Dec 25, 2024

Link to issue

https://reload.atlassian.net/browse/DDFBRA-290

Description

This PR handles:

  • Deletion of duplicate graphq users
  • Handling of separate consumers, users and roles for BNF and Go

and use consumer handler for entity deletion as well.
spaceo added 2 commits January 3, 2025 15:33
To avoid confusion we handle consumers and users in one place
Since the whole consumer/user/role part was refactored we also need to
adjust the drush commands handling secrets/uuids
Adjust cmds to fit new way of handling consumers and be more specific
about what each command actually is doing.
…brugeren-kun-bliver-oprettet-i-dpl-consumers-deploy-hook
@spaceo spaceo requested review from Dresse and rasben and removed request for Dresse and rasben January 5, 2025 12:15
@spaceo spaceo marked this pull request as ready for review January 5, 2025 12:15
Copy link
Contributor

@Dresse Dresse left a comment

Choose a reason for hiding this comment

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

Really nice work Mikkel 👍

I've went through the commits one by one, and gave the overall file changes a look as well. Do we need someone from the BNF team to take a look at this as well?

*
* This class is responsible for creating consumers and users.
*/
class ConsumerHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not fond of this class. What is a "ConsumerHandler" anyway?

On one hand it's a composition of consumer, user and role. On the other hand setComponents allows for setting new consumer, user and role.

In my mind it should either be a "handler", state free, and having create and delete just take the consumer, user and role as arguments.

Or it should be an object representing this combination (ConsumerSetup perhaps?), in which case it should take consumer, user and role as constructor arguments (and then you'd need a factory in order to get the container to inject the logger).

// Delete consume and users that we want to handle differently.
// We want to create consumers and consumer users
// specifically for the two known consumers (BNF and Go) and connected users.
(new Consumer("graphql_consumer", \Drupal::entityTypeManager()))->delete();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe ConsumerHandler should have methods for getting Consumer and ConsumerUser so the DI can be passed to it? Having to create a repository service for each of them is a bit overkill, but I think having to pass entityTypeManager to each new is a bit of a DX wart.

spaceo added 2 commits January 7, 2025 09:37
Since our deploy hook takes care of that now
My reviewer did not like setting the components as properties of the
handler.
In order to inject dependencies (right now entityTypeManager)
into  the consumer components (Consumer, ConsumerUser).
@spaceo spaceo force-pushed the DDFBRA-290-sorg-for-at-graph-ql-consumer-brugeren-kun-bliver-oprettet-i-dpl-consumers-deploy-hook branch from 34c8d7e to 2548ce1 Compare January 7, 2025 11:30
@spaceo spaceo requested a review from xendk January 7, 2025 11:30
Copy link
Contributor

@xendk xendk left a comment

Choose a reason for hiding this comment

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

👍

@spaceo spaceo merged commit 8155625 into develop Jan 7, 2025
19 checks passed
@spaceo spaceo deleted the DDFBRA-290-sorg-for-at-graph-ql-consumer-brugeren-kun-bliver-oprettet-i-dpl-consumers-deploy-hook branch January 7, 2025 11:55
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.

5 participants