-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Modifying comments tests #27446
Modifying comments tests #27446
Conversation
@SergioBertolinSG, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @DeepDiver1975 and @davitol to be potential reviewers. |
Had a quick glance, looks good 👍 |
Awesome, lets make CommentsApp great again! |
This step is commented because it is failing: https://github.com/owncloud/core/pull/27446/files#diff-6692878a78a86bbf0b3daf9e5e6522deR104 Raised here: #27455 |
|
Apparently tags tests depended on a comments function. I have moved it there. |
<d:propertyupdate xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns"> | ||
<d:set> | ||
<d:prop> | ||
<oc:message>'. $content .'</oc:message> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be safer to XML-encode this string to avoid injection.
Fortunately this is just test code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean escape? Is htmlspecialchars fine for this?
Fix the XML encode issue and this is good to go 👍 |
9e6e493
to
62f6d25
Compare
The property {http://owncloud.org/ns}comments-unread is not being included. Jenkins failure:
Seems to be a bug introduced in #27339 cc @mrow4a |
@SergioBertolinSG Did you check this scenario "by hand" first? Why this scenario talks about folder if this is a file? |
I checked it locally and it returned only the other two properties. |
Normal propfind works. Will look at it later, but this might not be working previously also, dont see a point in the code yet. |
@SergioBertolinSG this should be separate PR to this branch, isnt it? |
Ok, that failing test the only thing stopping this Pr to be merged now. |
@mrow4a please fix on this branch. CI will tell whether it was actually fixed. |
Fix problem with 0 comments
It occured, that when targeting single file with PROPFIND, it was returning null instead of 0, and requirement is to return the property. In web, it does not do propfinds to single files for unread, but this fix was needed anyways. |
👍 |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Refactoring and preparation for adding more tests related with comments.