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

Integrate BNF Article Import/Export Functionality. DDFHER-164 #1853

Merged
merged 17 commits into from
Dec 18, 2024
Merged

Conversation

rasben
Copy link
Contributor

@rasben rasben commented Dec 10, 2024

Please review commit by commit - I've tried my best to break it up, and describe it well here, and any summary I write here will just be confusing.

Locally, I've been testing with just enabling both bnf_client and bnf_server. As one of my @todo's say, we need to figure out how to actually work with this locally.
I've hardcoded that the server is just the same as the current site. This obviously also need to change :)

I'm also unsure about the authentication. I've added a few @todo's there with questions.
It has hardcoded username and password that only works locally, but, I didn't want to spend energy making it "proper" if we ended up doing something completely different.

In conclusion - this works, but, it's not necessarily what I had in mind when I started out. I'm a bit unsure about the GraphQL mutation, but still - it works as intended.

I'd love some help with my @todo's, but, I actually think I'd prefer if we just talk in person about those - maybe some pair-programming? :)

Feel free to test locally by enabling bnf_client and bnf_server.

Screenshot 2024-12-10 at 21 32 24

@rasben rasben changed the base branch from bnf-zero to develop December 10, 2024 20:10
This field will be used to keep track of which articles have been
imported to/from BNF already.
The importer will take the original node UUID and place it here.
Technically, this will be a read-only field, as we never want the user
to actually edit it, but I still decided to introduce it as a regular
node field as it will make debugging in the future much easier if we
have access to it, through the Drupal interface.
Creating a custom mutation graphQL endpoint, as part of the `bnf_server`
module.
This endpoint will receive a ping from client libraries, when they want
a certain node of theirs to be imported to BNF.
The client will send along a UUID and an endpoint which BNF can use to
do a GraphQL query against, to get the node data.
Introduce an importer service to the `bnf` common module.
This service takes the info that we received from the client ping, and
calls the supplied GraphQL source, parses the fields, and creates a
node.
This service will also be used in the future when the client wants to
import a peice of content from BNF.
Expanding the ImportRequest graphQL endpoint to actually attempt to
import the node that the client has supplied data about.
@rasben rasben changed the title Bnf send Integrate BNF Article Import/Export Functionality Dec 10, 2024
@rasben rasben changed the title Integrate BNF Article Import/Export Functionality Integrate BNF Article Import/Export Functionality. BNF-164 Dec 10, 2024
@rasben rasben changed the title Integrate BNF Article Import/Export Functionality. BNF-164 Integrate BNF Article Import/Export Functionality. DDFHER-164 Dec 10, 2024
@@ -3,6 +3,7 @@ langcode: en
status: true
dependencies:
config:
- field.field.node.article.field_bnf_uuid
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not need a field for this, Drupal already assigns an UUID for each piece of content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, this is the UUID field that we'll use across sites, to keep track of which content has been imported already

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, upstream uuid.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I don't like that it's just a regular field. It makes a hard coupling between the module and the site configuration. Never mind the fact that we don't need it on the server.

Sadly, I don't think it's possible to hook into entities base field definition. Have you looked into what contrib modules that need to add data to entities does?

Copy link
Contributor Author

@rasben rasben Dec 12, 2024

Choose a reason for hiding this comment

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

Never mind the fact that we don't need it on the server.

But.. we do need it on the server? It will be used when the libraries pull content from BNF? This is how we'll make sure that libraries cannot pull the same peice of content twice, nor that they can export something that they've imported

Sadly, I don't think it's possible to hook into entities base field definition. Have you looked into what contrib modules that need to add data to entities does?

I actually feel better about doing it this way, than adding a new dependency on another contrib module - but, if you have a suggestion for a field, I'm willing to hear it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It will be used when the libraries pull content from BNF

Server side, it's just the node UUID. What we need is an identifier that identifies a node on the server. As nodes are born with a UUID, we already have that, hell, as it only needs to be unique on the server we could use nid.

But if we sync UUID when we sync nodes, we don't even have to store the "original UUID" as the same content on different sites will have the same UUID (nid wont work for this). The only data we then need to store is the fact that this node was imported/exported, and that's just to be able to make a list of imported content.

I actually feel better about doing it this way, than adding a new dependency on another contrib module - but, if you have a suggestion for a field, I'm willing to hear it :)

Wasn't suggesting you find a contrib module that does it for you, but that you looked into how they do it. scheduler would be a good candidate, it needs to add dates to all nodes, but they're not fields in the configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated it now: 3f1c7c3

Notice that I've decided to still introduce the "field" both on client and server side. This is on purpose, to avoid having to alter the code, as we both need to be able to import articles on both servers and clients.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is on purpose, to avoid having to alter the code, as we both need to be able to import articles on both servers and clients.

What you've accomplished is that we now have two UUID fields on both sides, which people are bound to confuse. If we just use and sync node UUID we don't need to know what was synced, but that it was synced. Actually, for all intent and purposes, the syncing code doesn't even care about that, all it needs to know is that if two UUIDs match, it's the same content, and it should (potentially) update the receiver.

The only reason why we're interested in that it's synced is UI wise, to make lists of "stuff imported" and that can be handled with a simple bnf_status int. 0 = no BNF, 1 = incoming (imported from BNF client side, library submitted content server side) and 2 = outgoing (client side "send to BNF", not used server side). Code wise it's also much simpler to make a "list of imported content" by using WHERE bnf_status = 1 than WHERE bnf_uuid IS NOT NULL AND uuid != bnf_uuid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've introduced bnf_state now, as we've talked about.

web/modules/custom/bnf/src/Services/BnfImporter.php Outdated Show resolved Hide resolved
phpstan.neon.dist Show resolved Hide resolved
web/modules/custom/bnf/bnf.info.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
A service with a single method, that is used to programmatically connect
to the BNF ImportRequest graphQL endpoint.
Adding a simple form field to the node edit form, with a checkbox.
If the checkbox is checked, the node will be published and sent to BNF
upon save.
Originally, I wanted to have a "publish to BNF" button, but that became
very complicated, as I'd have to replicate some of the submit handling
that happens with the core "submit" button.
In the end, I think this solution is user friendly enough.
For debugging sakes, I prefer not to hide modules in the interface.
None of our BNF modules will be enabled manually, regardless.
This uses environment variables, that we also need to set on lagoon.
Ontop of that, we set the GraphQL calls to be HTTPS.
This introduces a 'hidden' field, that cannot be manipulated
by the editors. Apart from that, it works the same way as a
usual field, programmatically.
The field lets us associate a state with the node, so we can
see if the node has been imported or exported.
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.

👍

@rasben rasben merged commit b57362b into develop Dec 18, 2024
19 checks passed
@rasben rasben deleted the bnf-send branch December 18, 2024 20:36
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.

3 participants