harmen/hypha
1
0
Fork 0
Code Issues 142 Pull requests 4 Projects Releases Packages Wiki Activity

Updated peer reviewed article to new standard. #258

Merged
laurensmartina merged 5 commits from newtext into master 2019-11-20 00:20:57 +00:00
Conversation 27 Commits 5 Files changed 8 +1180 -1426
laurensmartina commented 2019-06-20 13:43:29 +00:00 (Migrated from github.com)
Copy link
No description provided.
giplt (Migrated from github.com) reviewed 2019-06-20 13:43:29 +00:00
tammoterhark (Migrated from github.com) reviewed 2019-06-20 13:43:29 +00:00
RoukePouw (Migrated from github.com) reviewed 2019-06-20 13:43:29 +00:00
dianawi (Migrated from github.com) reviewed 2019-06-20 13:43:29 +00:00
matthijskooijman (Migrated from github.com) reviewed 2019-07-02 10:59:30 +00:00
matthijskooijman (Migrated from github.com) left a comment
Copy link

I've reviewed up to approveAction(). Nothing blocking so far, just some thoughts for improvements. I'll try to review the rest ASAP.

I've reviewed up to `approveAction()`. Nothing blocking so far, just some thoughts for improvements. I'll try to review the rest ASAP.
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-02 09:31:43 +00:00
Copy link

Possible improvement: Should status change and approve need their own path? Or would it make more sense to just use a null path for them?

Possible improvement: Should status change and approve need their own path? Or would it make more sense to just use a null path for them?
system/datatypes/peer_reviewed_article.php
@ -468,0 +235,4 @@
$discussions = $this->xml->find(self::FIELD_NAME_DISCUSSION_PUBLIC_CONTAINER);
/** @var NodeList $commentCollection */
$commentCollection = $discussions->findXPath('.//' . self::FIELD_NAME_DISCUSSION_COMMENT . '[not(@pending="1")]');
matthijskooijman (Migrated from github.com) commented 2019-07-02 09:44:25 +00:00
Copy link

I wonder if this should use ->children() rather than ->getHtml(). AFAICS getHtml() serializes into a HTML string, which is not really needed. Same applies for similar lines below.

I wonder if this should use `->children()` rather than `->getHtml()`. AFAICS `getHtml()` serializes into a HTML string, which is not really needed. Same applies for similar lines below.
matthijskooijman (Migrated from github.com) commented 2019-07-02 10:07:37 +00:00
Copy link

I was a bit confused by the appendChildrenToDiscussionsDomElement method, I somehow expected it to add $discussionsDomElement somewhere into the HTML DOM (though I now see that happens in the next line). In reality, the method only adds existing comments to the container. Perhaps it should be named differently? addDiscussionsToElement, or fillDiscussionsContainer or something like that?

I was a bit confused by the `appendChildrenToDiscussionsDomElement` method, I somehow expected it to add `$discussionsDomElement` somewhere into the HTML DOM (though I now see that happens in the next line). In reality, the method only adds existing comments to the container. Perhaps it should be named differently? `addDiscussionsToElement`, or `fillDiscussionsContainer` or something like that?
matthijskooijman (Migrated from github.com) commented 2019-07-02 10:55:19 +00:00
Copy link

This setHTML call seems a bit opaque. I think the true is intended to wikify, but that should probably be commented, or perhaps use a new setWikifiedHtml() method? Also, it seems that setHTML() with two arguments was added by DocPage, which is deleted by this commit? Does this code actually work right now?

This setHTML call seems a bit opaque. I think the `true` is intended to wikify, but that should probably be commented, or perhaps use a new `setWikifiedHtml()` method? Also, it seems that `setHTML()` with two arguments was added by `DocPage`, which is deleted by this commit? Does this code actually work right now?
matthijskooijman (Migrated from github.com) commented 2019-07-02 10:57:18 +00:00
Copy link

Maybe a $request->getArgument() would be better? That emphasizes the special nature of this argument argument.

Maybe a `$request->getArgument()` would be better? That emphasizes the special nature of this `argument` argument.
system/datatypes/peer_reviewed_article.php
@ -1022,2 +761,3 @@
/**
* @param DocPage $discussion
* Creates a discussion DOM element.
*
matthijskooijman (Migrated from github.com) commented 2019-07-02 09:57:32 +00:00
Copy link

This uses $list, which is reused below, but they are essentially distinct variables (since either this if, or the below loop runs but never both). Maybe using different variable names helps to clarify the distinctness of these variables.

This uses `$list`, which is reused below, but they are essentially distinct variables (since either this if, or the below loop runs but never both). Maybe using different variable names helps to clarify the distinctness of these variables.
matthijskooijman (Migrated from github.com) commented 2019-07-02 10:03:52 +00:00
Copy link

Alternatively, the variable could be removed entirely by using the (recently added) appendTo() from DomWrapper:

$this->html->createElement('ul')->append('<li>' . __('art-no-comments-yet') . '</li>')->appendTo($discussionsContainer);

Or slightly shorter:

    $this->html->create('ul')->append('<li>' . __('art-no-comments-yet') . '</li>')->appendTo($discussionsContainer);

(note that create can also accept more complete HTML snippets, unlike createElement). Perhaps the ul above must be <ul> for create to accept it, but I think just ul should also work.

One further variation would be:

    $this->html->create('ul')->append($this->html->create('li')->text(__('art-no-comments-yet')))->appendTo($discussionsContainer);

But I'm not sure if that's really more readable (it does subtly fix escaping issues, though, and reduces the amount of HTML parsing needed). Might be better when the li element is put in a separate variable perhaps.

Alternatively, the variable could be removed entirely by using the (recently added) `appendTo()` from DomWrapper: $this->html->createElement('ul')->append('<li>' . __('art-no-comments-yet') . '</li>')->appendTo($discussionsContainer); Or slightly shorter: $this->html->create('ul')->append('<li>' . __('art-no-comments-yet') . '</li>')->appendTo($discussionsContainer); (note that `create` can also accept more complete HTML snippets, unlike `createElement`). Perhaps the `ul` above must be `<ul>` for `create` to accept it, but I think just `ul` should also work. One further variation would be: $this->html->create('ul')->append($this->html->create('li')->text(__('art-no-comments-yet')))->appendTo($discussionsContainer); But I'm not sure if that's really more readable (it does subtly fix escaping issues, though, and reduces the amount of HTML parsing needed). Might be better when the `li` element is put in a separate variable perhaps.
system/datatypes/peer_reviewed_article.php
@ -1029,0 +802,4 @@
$comment = $this->xml->createElement(self::FIELD_NAME_DISCUSSION_COMMENT);
$comment->generateId();
$discussion->append($comment);
matthijskooijman (Migrated from github.com) commented 2019-07-02 09:41:22 +00:00
Copy link

The comment does not match the variablename (existing problem, though).

The comment does not match the variablename (existing problem, though).
matthijskooijman (Migrated from github.com) reviewed 2019-07-09 13:41:43 +00:00
matthijskooijman (Migrated from github.com) left a comment
Copy link

And finished the review :-)

I left quite some more remarks, but nothing really critical I believe. Most of my comments are easy fixes, so I would suggest fixing those before merging. Some comments are a bit more involved, so feel free to leave these for later (or fix them right away if you feel like that, of course). If you need input on anything specific, just tag me in a comment and I'll have another look.

Once you have made another pass over this and are happy with it, I suggest we merge it, put it on test.destadsbron.nl and do some more elaborate testing there (also with existing content).

And finished the review :-) I left quite some more remarks, but nothing really critical I believe. Most of my comments are easy fixes, so I would suggest fixing those before merging. Some comments are a bit more involved, so feel free to leave these for later (or fix them right away if you feel like that, of course). If you need input on anything specific, just tag me in a comment and I'll have another look. Once you have made another pass over this and are happy with it, I suggest we merge it, put it on test.destadsbron.nl and do some more elaborate testing there (also with existing content).
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 12:59:05 +00:00
Copy link

Maybe rename this to CMD_START_DISCUSSION?

Maybe rename this to `CMD_START_DISCUSSION`?
system/datatypes/peer_reviewed_article.php
@ -468,0 +235,4 @@
$discussions = $this->xml->find(self::FIELD_NAME_DISCUSSION_PUBLIC_CONTAINER);
/** @var NodeList $commentCollection */
$commentCollection = $discussions->findXPath('.//' . self::FIELD_NAME_DISCUSSION_COMMENT . '[not(@pending="1")]');
matthijskooijman (Migrated from github.com) commented 2019-07-09 11:13:52 +00:00
Copy link

I think this $type variable should be validated/checked.

I think this `$type` variable should be validated/checked.
system/datatypes/peer_reviewed_article.php
@ -482,0 +604,4 @@
$form = $this->createCommentForm($type, $request->getPostData(), $discussion);
if (!$review && !isUser()) {
$form->validateRequiredField(self::FIELD_NAME_DISCUSSION_COMMENTER_NAME . $dataFieldSuffix);
$form->validateRequiredField(self::FIELD_NAME_DISCUSSION_COMMENTER_EMAIL . $dataFieldSuffix);
matthijskooijman (Migrated from github.com) commented 2019-07-09 11:34:47 +00:00
Copy link

This should escape/encode the code before inserting into Xpath. There is a function available here, you should probably pull that commit into this PR and use it. Alternatively, we can leave this for now (I've created https://github.com/PlanBCode/hypha/issues/263 for the issue in general).

This should escape/encode the code before inserting into Xpath. There is a function available [here](https://github.com/PlanBCode/hypha/pull/229/commits/12973e426c8cbd88d2c3d6c3540be6be0413f28e), you should probably pull that commit into this PR and use it. Alternatively, we can leave this for now (I've created https://github.com/PlanBCode/hypha/issues/263 for the issue in general).
system/datatypes/peer_reviewed_article.php
@ -1022,2 +761,3 @@
/**
* @param DocPage $discussion
* Creates a discussion DOM element.
*
matthijskooijman (Migrated from github.com) commented 2019-07-09 11:26:21 +00:00
Copy link

Maybe it would be clearer to rename this function to createStartDiscussionForm (to better distinguish it from the createDiscussionCommentForm)?

Maybe it would be clearer to rename this function to `createStartDiscussionForm` (to better distinguish it from the `createDiscussionCommentForm`)?
matthijskooijman (Migrated from github.com) commented 2019-07-09 12:21:13 +00:00
Copy link

This variable and CSS class talk about "review", but this code also handles public comments.

This variable and CSS class talk about "review", but this code also handles public comments.
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 11:12:54 +00:00
Copy link

This comment is no longer accurate.

This comment is no longer accurate.
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 11:23:41 +00:00
Copy link

I wonder if this query should be limited to the list of discussions. AFAICS you can now pass an id of a comment or approve element, and then the XML structure will be messed up (which is possibly a DoS vulnerability or worse). This might be out of scope for this PR though (maybe just add a TODO comment in the code).

I wonder if this query should be limited to the list of discussions. AFAICS you can now pass an id of a comment or approve element, and then the XML structure will be messed up (which is possibly a DoS vulnerability or worse). This might be out of scope for this PR though (maybe just add a TODO comment in the code).
matthijskooijman (Migrated from github.com) commented 2019-07-09 11:29:00 +00:00
Copy link

Note that this also holds for other calls to getElementById elsewhere.

Note that this also holds for other calls to `getElementById` elsewhere.
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 11:43:31 +00:00
Copy link

This should probably read "Creates a discussion DOM element inside the given container and fills it with data from the submitted form passed."

This should probably read "Creates a discussion DOM element inside the given container and fills it with data from the submitted form passed."
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 11:45:45 +00:00
Copy link

All this stuff about blocking and closed, shouldn't that be only for review discussions?

All this stuff about blocking and closed, shouldn't that be only for review discussions?
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 11:46:59 +00:00
Copy link

This should probably read "Creates a comment DOM element inside the given discussion and fills it with data from the submitted form passed."

This should probably read "Creates a comment DOM element inside the given discussion and fills it with data from the submitted form passed."
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 11:53:30 +00:00
Copy link

This should probably read "Creates the approves DOM element and fills it with a list of previous approvals."

This should probably read "Creates the approves DOM element and fills it with a list of previous approvals."
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 11:57:19 +00:00
Copy link

This is not terribly easy to read. Maybe using usort on $discussions before looping is cleaner and easier to understand?

This is not terribly easy to read. Maybe using usort on `$discussions` before looping is cleaner and easier to understand?
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 12:02:15 +00:00
Copy link

Huh? When is $comments ever an array here? It seems this is a remnant from when DocPage::getChildren() was used which returns a nested array of children sorted by tagname or something like that. Now this uses the regular children() method, so I think this check, as well as the loop over $comments below can be removed. Additionally, $firstComment = true should be moved up one loop then.

Huh? When is $comments ever an array here? It seems this is a remnant from when DocPage::getChildren() was used which returns a nested array of children sorted by tagname or something like that. Now this uses the regular `children()` method, so I think this check, as well as the loop over `$comments` below can be removed. Additionally, `$firstComment = true` should be moved up one loop then.
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 12:19:53 +00:00
Copy link

Maybe add "(when all comments are still pending)" here?

Maybe add "(when all comments are still pending)" here?
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 12:24:35 +00:00
Copy link

I wonder if this should/could use the same interpolate / replace function as translations. Might be overkill, though. Maybe at some point an URL-specific interpolation / building function might make sense (that can also handle urlencoding and/or decoding when needed)

I wonder if this should/could use the same interpolate / replace function as translations. Might be overkill, though. Maybe at some point an URL-specific interpolation / building function might make sense (that can also handle urlencoding and/or decoding when needed)
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-07-09 13:36:13 +00:00
Copy link

This probably needs nl2br.

This probably needs `nl2br`.
laurensmartina (Migrated from github.com) reviewed 2019-11-11 14:08:32 +00:00
system/datatypes/peer_reviewed_article.php
@ -1029,0 +802,4 @@
$comment = $this->xml->createElement(self::FIELD_NAME_DISCUSSION_COMMENT);
$comment->generateId();
$discussion->append($comment);
laurensmartina (Migrated from github.com) commented 2019-11-11 14:08:31 +00:00
Copy link

fixed it

fixed it
laurensmartina (Migrated from github.com) reviewed 2019-11-11 14:10:03 +00:00
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2019-11-11 14:10:02 +00:00
Copy link

removed the paths

removed the paths
laurensmartina (Migrated from github.com) reviewed 2019-11-11 15:09:50 +00:00
system/datatypes/peer_reviewed_article.php
@ -468,0 +235,4 @@
$discussions = $this->xml->find(self::FIELD_NAME_DISCUSSION_PUBLIC_CONTAINER);
/** @var NodeList $commentCollection */
$commentCollection = $discussions->findXPath('.//' . self::FIELD_NAME_DISCUSSION_COMMENT . '[not(@pending="1")]');
laurensmartina (Migrated from github.com) commented 2019-11-11 15:09:49 +00:00
Copy link

done

done
laurensmartina (Migrated from github.com) reviewed 2019-11-11 15:35:02 +00:00
system/datatypes/peer_reviewed_article.php
@ -1022,2 +761,3 @@
/**
* @param DocPage $discussion
* Creates a discussion DOM element.
*
laurensmartina (Migrated from github.com) commented 2019-11-11 15:35:02 +00:00
Copy link

Found out that text() does not return self.
Implemented like:

$noCommentContainer = $this->html->createElement('ul')->appendTo($discussionsContainer);
$noComment = $this->html->create('li')->appendTo($noCommentContainer);
$noComment->text(__('art-no-comments-yet'));
Found out that `text()` does not return `self`. Implemented like: ``` $noCommentContainer = $this->html->createElement('ul')->appendTo($discussionsContainer); $noComment = $this->html->create('li')->appendTo($noCommentContainer); $noComment->text(__('art-no-comments-yet')); ```
laurensmartina (Migrated from github.com) reviewed 2019-11-11 16:01:03 +00:00
system/datatypes/peer_reviewed_article.php
@ -468,0 +235,4 @@
$discussions = $this->xml->find(self::FIELD_NAME_DISCUSSION_PUBLIC_CONTAINER);
/** @var NodeList $commentCollection */
$commentCollection = $discussions->findXPath('.//' . self::FIELD_NAME_DISCUSSION_COMMENT . '[not(@pending="1")]');
laurensmartina (Migrated from github.com) commented 2019-11-11 16:01:03 +00:00
Copy link

changed name to fillDiscussionsContainer

changed name to `fillDiscussionsContainer`
laurensmartina (Migrated from github.com) reviewed 2019-11-11 16:27:45 +00:00
system/datatypes/peer_reviewed_article.php
@ -468,0 +235,4 @@
$discussions = $this->xml->find(self::FIELD_NAME_DISCUSSION_PUBLIC_CONTAINER);
/** @var NodeList $commentCollection */
$commentCollection = $discussions->findXPath('.//' . self::FIELD_NAME_DISCUSSION_COMMENT . '[not(@pending="1")]');
laurensmartina (Migrated from github.com) commented 2019-11-11 16:27:45 +00:00
Copy link

fixed

fixed
laurensmartina (Migrated from github.com) reviewed 2019-11-11 17:20:29 +00:00
system/datatypes/peer_reviewed_article.php
@ -468,0 +235,4 @@
$discussions = $this->xml->find(self::FIELD_NAME_DISCUSSION_PUBLIC_CONTAINER);
/** @var NodeList $commentCollection */
$commentCollection = $discussions->findXPath('.//' . self::FIELD_NAME_DISCUSSION_COMMENT . '[not(@pending="1")]');
laurensmartina (Migrated from github.com) commented 2019-11-11 17:20:29 +00:00
Copy link

made improvements

made improvements
laurensmartina (Migrated from github.com) reviewed 2019-11-12 07:53:12 +00:00
system/datatypes/peer_reviewed_article.php
@ -468,0 +235,4 @@
$discussions = $this->xml->find(self::FIELD_NAME_DISCUSSION_PUBLIC_CONTAINER);
/** @var NodeList $commentCollection */
$commentCollection = $discussions->findXPath('.//' . self::FIELD_NAME_DISCUSSION_COMMENT . '[not(@pending="1")]');
laurensmartina (Migrated from github.com) commented 2019-11-12 07:53:12 +00:00
Copy link

done

done
laurensmartina (Migrated from github.com) reviewed 2019-11-12 08:31:58 +00:00
system/datatypes/peer_reviewed_article.php
@ -1022,2 +761,3 @@
/**
* @param DocPage $discussion
* Creates a discussion DOM element.
*
laurensmartina (Migrated from github.com) commented 2019-11-12 08:31:58 +00:00
Copy link

done

done
laurensmartina (Migrated from github.com) reviewed 2019-11-12 08:36:48 +00:00
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2019-11-12 08:36:47 +00:00
Copy link

@matthijskooijman, we should create a separate issue for this. I regard this as "out of scope" for now.

@matthijskooijman, we should create a separate issue for this. I regard this as "out of scope" for now.
matthijskooijman (Migrated from github.com) reviewed 2019-11-17 16:24:10 +00:00
system/datatypes/peer_reviewed_article.php
matthijskooijman (Migrated from github.com) commented 2019-11-17 16:24:10 +00:00
Copy link

Done, #270

Done, #270
laurensmartina (Migrated from github.com) reviewed 2019-11-18 15:53:23 +00:00
system/datatypes/peer_reviewed_article.php
@ -482,0 +604,4 @@
$form = $this->createCommentForm($type, $request->getPostData(), $discussion);
if (!$review && !isUser()) {
$form->validateRequiredField(self::FIELD_NAME_DISCUSSION_COMMENTER_NAME . $dataFieldSuffix);
$form->validateRequiredField(self::FIELD_NAME_DISCUSSION_COMMENTER_EMAIL . $dataFieldSuffix);
laurensmartina (Migrated from github.com) commented 2019-11-18 15:53:23 +00:00
Copy link

done

done
laurensmartina (Migrated from github.com) reviewed 2019-11-18 16:01:23 +00:00
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2019-11-18 16:01:23 +00:00
Copy link

current text covers all (future) cases

current text covers all (future) cases
laurensmartina (Migrated from github.com) reviewed 2019-11-18 16:04:17 +00:00
system/datatypes/peer_reviewed_article.php
@ -1022,2 +761,3 @@
/**
* @param DocPage $discussion
* Creates a discussion DOM element.
*
laurensmartina (Migrated from github.com) commented 2019-11-18 16:04:17 +00:00
Copy link

good catch!

good catch!
laurensmartina (Migrated from github.com) reviewed 2019-11-18 16:33:40 +00:00
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2019-11-18 16:33:39 +00:00
Copy link

out of scope for this pull request

out of scope for this pull request
laurensmartina (Migrated from github.com) reviewed 2019-11-18 16:37:15 +00:00
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2019-11-18 16:37:14 +00:00
Copy link

Renamed to CMD_DISCUSSION_STARTED

Renamed to `CMD_DISCUSSION_STARTED`
laurensmartina (Migrated from github.com) reviewed 2019-11-18 16:37:49 +00:00
system/datatypes/peer_reviewed_article.php
laurensmartina (Migrated from github.com) commented 2019-11-18 16:37:49 +00:00
Copy link

done

done
matthijskooijman (Migrated from github.com) reviewed 2019-11-18 16:46:37 +00:00
system/datatypes/peer_reviewed_article.php
@ -482,0 +659,4 @@
$userId = $this->O_O->getUser()->getAttribute('id');
$blocking = (bool)$discussion->attr(self::FIELD_NAME_DISCUSSION_BLOCKING);
matthijskooijman (Migrated from github.com) commented 2019-11-18 16:46:37 +00:00
Copy link

Maybe this should be:

'field-name-title' => self::FIELD_NAME_TITLE,

Then both sides are a bit more consistent with each other? If so, we should change that in text as well (I'll do that, then).

Maybe also use the same syntax (dash-separated parts rather than camelCase) for the other variables?

Maybe this should be: 'field-name-title' => self::FIELD_NAME_TITLE, Then both sides are a bit more consistent with each other? If so, we should change that in text as well (I'll do that, then). Maybe also use the same syntax (dash-separated parts rather than camelCase) for the other variables?
laurensmartina (Migrated from github.com) reviewed 2019-11-18 17:00:22 +00:00
system/datatypes/peer_reviewed_article.php
@ -482,0 +659,4 @@
$userId = $this->O_O->getUser()->getAttribute('id');
$blocking = (bool)$discussion->attr(self::FIELD_NAME_DISCUSSION_BLOCKING);
laurensmartina (Migrated from github.com) commented 2019-11-18 17:00:21 +00:00
Copy link

done

done
matthijskooijman commented 2019-11-19 10:58:41 +00:00 (Migrated from github.com)
Copy link

I've removed the textpage commits from this PR, since they are essentially unrelated and part of #272.

I've removed the textpage commits from this PR, since they are essentially unrelated and part of #272.
matthijskooijman (Migrated from github.com) approved these changes 2019-11-19 12:07:20 +00:00
matthijskooijman (Migrated from github.com) left a comment
Copy link

I've extracted the xpath_encode function into a separate commit and force-pushed that.

I've done another fairly shallow review of the code and some quick testing, and it seems good for merging. Shall we just merge this as-is now, or maybe first put it on test.destadsbron.nl first? @laurensmartina, did you already look at publishing this on test?

I've extracted the `xpath_encode` function into a separate commit and force-pushed that. I've done another fairly shallow review of the code and some quick testing, and it seems good for merging. Shall we just merge this as-is now, or maybe first put it on test.destadsbron.nl first? @laurensmartina, did you already look at publishing this on test?
Sign in to join this conversation.
Reviewers
No reviewers
RoukePouw
tammoterhark
giplt
dianawi
matthijskooijman
Labels
Clear labels   
Component: User interface

   
Component: Wymeditor

   
Help wanted

   
Level: Difficult

Structural changes, need discussion

  
Level: Easy

Under 4 hours of work

  
Level: Moderate

Under 16 hours of work

  
Pagetype: Festival

   
Pagetype: Mailinglist

   
Pagetype: Peer reviewed article

   
Pagetype: Text

   
Privacy GDPR AVG

   
status: has conflicts

   
Status: Needs changes

   
Status: Needs discussion

   
Status: Needs review

   
Status: Ready to merge

   
Status: Waiting for response

   
Type: Bug

   
Type: Enhancement

   
Type: Question

   
Usecase: De Stadsbron

   
Usecase: Koppelting

   
Usecase: MeetjeStad

   
Value: Coders

Enhancement of codebase, API, documentation

  
Value: Security

   
Value: Users

Improve user experience, add new features

  
Value: Visitors

Improve navigation, styling

No labels
Component: User interface
Component: Wymeditor
Help wanted
Level: Difficult
Level: Easy
Level: Moderate
Pagetype: Festival
Pagetype: Mailinglist
Pagetype: Peer reviewed article
Pagetype: Text
Privacy GDPR AVG
status: has conflicts
Status: Needs changes
Status: Needs discussion
Status: Needs review
Status: Ready to merge
Status: Waiting for response
Type: Bug
Type: Enhancement
Type: Question
Usecase: De Stadsbron
Usecase: Koppelting
Usecase: MeetjeStad
Value: Coders
Value: Security
Value: Users
Value: Visitors
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
harmen/hypha!258
No description provided.