-
Notifications
You must be signed in to change notification settings - Fork 328
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/allow single submissions and path names #1935
Feature/allow single submissions and path names #1935
Conversation
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.
As mentioned on the last PR, since you are changing/adding features on how submissions can be given to Jplag, please add an e2e case for each of them.
Here are references on where they are implemented currently:
develop/report-viewer/tests/e2e/OpenComparisonTest.spec.ts
develop/.github/workflows/complete-e2e.yml
Please add one for every Problem solved by this PR
Ok, the tests I did so far weren't in the e2e, I'll add some |
The functioning of the e2e tests seems to be more complicated than the normal Java tests.
Is it correct? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Quality Gate passed for 'JPlag Plagiarism Detector'Issues Measures |
In this check, the error is Otherwise all checks seem to have passed! |
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.
I finally found some time to take a look at the PR. Understanding what was part of this PR and what was not was a bit tricky due to the lack of a PR description.
If I understand correctly, this PR currently contains two changes:
- Fix an issue where single-submission root directory + single-submission old directory was not accepted
- Fix the issue with multiple root folders that have the same name by using the shortest uncommon path
We discussed these features internally. The first one is excellent; we would like to include it. We currently do not want to support the second one, as including super directories in the root names exposes them in the report zip. This means a user could expose information about their system via a report that might not be intended.
Sorry for giving you this feedback so late.
Ok, so should I divide the pull request in two pull requests? |
Also for the second feature, we could make it that it stops until the current dir where it is executed JPlag CLI or something similar so that it doesn't give further than needed |
Yes, the first feature should be its own PR.
I will take that idea into our internal discussions. |
Ok, then:
I think with a specific flag to enable the feature the security issue is solved, because one that wants to use it must explicitly specify that the behaviour is wanted |
Closed, as discussed above. |
This pull request add minor updates described in the last comment of #1852. It fixed #1848 and #1849