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

Feature/regulatory statements #325

Merged
merged 48 commits into from Oct 5, 2022
Merged

Conversation

nvdk
Copy link
Member

@nvdk nvdk commented Sep 12, 2022

reworking #317 a bit for a cleaner diff, needs lblod/app-gelinkt-notuleren#111 as a backend

@nvdk nvdk changed the base branch from master to futurenow September 12, 2022 12:52
@nvdk nvdk changed the base branch from futurenow to master September 12, 2022 12:54
@nvdk nvdk force-pushed the feature/regulatory-statements branch from 0b5ffc0 to 6cabc35 Compare September 12, 2022 14:52
@@ -81,7 +81,7 @@ export default class DocumentCreatorComponent extends Component {
if (this.template) {
if (this.template.reload) {
// regular templates from templatesForContext do not return body of template
await this.template.reload();
await this.template.reload(this.template);
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

aah but here's the fun part
template is not an ember-data model here
look at the RegulatoryAttachmentsFetcher service
it makes a raw sparql request and from the bindings it creates an object which does have a reload method that takes an argument

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes sorry, forgot about this comment, basically I wanted to make it compatible with the old document creator and it was the best way I came up with

Copy link
Contributor

Choose a reason for hiding this comment

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

If anyone has a better idea feel free to suggest it

Copy link
Member Author

Choose a reason for hiding this comment

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

but, why? we already check if the object has a reload function and only execute reload then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because I need a hook to fetch the template content, so I don't fetch all the contents before the user selects the one he wants to use.

@@ -51,8 +53,14 @@ export default class RegulatoryAttachmentsFetcher extends Service {
const templates = bindings.map((binding) => ({
container: binding.container.value,
title: binding.title.value,
reload: async (template) => {
const response = await fetch(
`${config.regulatoryStatementPreviewEndpoint}/${binding.uuid.value}`
Copy link
Member Author

Choose a reason for hiding this comment

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

as discussed I think this should use the regular file service

Copy link
Member

Choose a reason for hiding this comment

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

come on it's the year 2022
the original regex was too loose, it picked up variable specs of the form ${name}
and then tried to evaluate "name".

Replaced with an exact match for ${generateUuid()} since that's all we support anyway.
to avoid npm woes from overly strict peerdep on editor
Repair basic RB registry integration
Copy link
Contributor

@lagartoverde lagartoverde left a comment

Choose a reason for hiding this comment

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

Works as expected, but the table of contents plugin is not updating at all

@elpoelma
Copy link
Contributor

elpoelma commented Oct 4, 2022

Works as expected, but the table of contents plugin is not updating at all

I will look into it.

@elpoelma
Copy link
Contributor

elpoelma commented Oct 4, 2022

The issue with the table of contents not updating with the template content lies in the article-structure-plugin. The main issue is that two different divs with the type say:DocumentContent are created:

  • one div with the structure of the template
  • one div with the 'Insert here' text
    The problem is that the first div with say:DocumentContent does not contain the necessary prefixes for the editor to resolve the rdfa of its content, that is why the table of contents can not detect it.

@elpoelma
Copy link
Contributor

elpoelma commented Oct 4, 2022

This PR for the registry: lblod/frontend-reglementaire-bijlage#27 adds new prefixes to the root of the document so the rdfa content is correctly resolved. It also changes the type-uri for a reglementaire bijlage.

Copy link
Member

@abeforgit abeforgit left a comment

Choose a reason for hiding this comment

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

doesnt work (anymore)
creating a template errors out
and this is with setting the endpoints correctly to the dev server

@elpoelma
Copy link
Contributor

elpoelma commented Oct 4, 2022

doesnt work (anymore) creating a template errors out and this is with setting the endpoints correctly to the dev server

Locally it seems to work for me, what specific errors do you get? JSON parsing errors?

@abeforgit
Copy link
Member

yep my bad, set up the wrong preview endpoint

@abeforgit abeforgit self-requested a review October 4, 2022 19:23
@nvdk
Copy link
Member Author

nvdk commented Oct 5, 2022

so I understand correctly the outstanding issues (TOC not updating) is an issue in the registry and this is ready to be merged?

@elpoelma
Copy link
Contributor

elpoelma commented Oct 5, 2022

so I understand correctly the outstanding issues (TOC not updating) is an issue in the registry and this is ready to be merged?

The issue is also solved in the registry and is merged.


async model(params) {
const options = {
sort: params.sort,
Copy link
Member

Choose a reason for hiding this comment

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

this view should sort on date created by default
https://binnenland.atlassian.net/browse/GN-3735 tracked here if too much work

Copy link
Member

Choose a reason for hiding this comment

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

@abeforgit
Copy link
Member

abeforgit commented Oct 5, 2022

about the endpoints, do we add the dev backends in the dev env like we do for roadsigns?
I could see it being annoying if you want to override that backend with an env var on your local, but on the other hand, how often would that happen anyway

edit: this is done now

the template referenced all kinds of stuff that should be on the controller, except there was no controller
the param was never passed to the query, so the value was never used. We were lucky the default size of 20 seems like a sensible amount.
Fix sorting, search and pagination on RS overview page
@abeforgit abeforgit merged commit 4632e9b into master Oct 5, 2022
@abeforgit abeforgit deleted the feature/regulatory-statements branch October 5, 2022 18:34
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 this pull request may close these issues.

None yet

5 participants