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.

Sign up for GitHub

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

Jump to bottom

[23.0] Fixes for (gitlab) error reporting #16424

Merged
merged 4 commits into from Jul 26, 2023
Merged

[23.0] Fixes for (gitlab) error reporting #16424

merged 4 commits into from Jul 26, 2023

Conversation

bernt-matthias
Copy link
Contributor

@bernt-matthias bernt-matthias commented Jul 20, 2023 •
edited

Fixes the gitlab error reporting plugin (for the case that comments should be added to existing issues if possible) and also fixes the display of the message of the error reporters to the users (which should not be quoted).

@davelopez I could not get your suggested markdown solution running.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added this to the 23.0 milestone Jul 20, 2023
@hexylena
Copy link
Member

fyi @selten who was mostly maintaining this plugin

@davelopez
Copy link
Contributor

@bernt-matthias, what was exactly the issue with the markdown solution? Maybe I can help.

mvdbeek
mvdbeek reviewed Jul 21, 2023
View reviewed changes
client/src/components/DatasetInformation/DatasetError.vue Outdated
@@ -56,10 +56,11 @@
<h4 class="mb-3 h-md">Issue Report</h4>
<b-alert
v-for="(resultMessage, index) in resultMessages"
v-html="resultMessage[0]"
Copy link
Member

Choose a reason for hiding this comment

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

I think the resultMessage can potentially contain user-generated content, so that can open up XSS vulnerabilities and/or mess with the component layout.

@bernt-matthias
Copy link
Contributor Author

bernt-matthias commented Jul 21, 2023 •
edited

Just re-tried with the changes in cb098e9 and got this:

Screenshot from 2023-07-21 10-38-21

Ups: actually I tried without the v-html as fixed in 851b176

@bernt-matthias
Copy link
Contributor Author

I guess the error reporters just have to output markdown instead of HTML, isn't it .. lets try

@davelopez
Copy link
Contributor

I guess the error reporters just have to output markdown instead of HTML, isn't it

Yes, exactly :)

@bernt-matthias
Copy link
Contributor Author

Still quoted. Also the <p>...</p> tags surrounding everything (apparently added by the markdown engine) are quoted.

@davelopez
Copy link
Contributor

davelopez commented Jul 21, 2023 •
edited

Where is the <p> coming from? I guess you just need to remove it from the templates and make sure they are all in Markdown.

@bernt-matthias
Copy link
Contributor Author

bernt-matthias commented Jul 21, 2023 •
edited

I guess from https://markdown-it.github.io/markdown-it/#MarkdownIt.render .. since the docs for https://markdown-it.github.io/markdown-it/#MarkdownIt.renderInline remark that this does not add <p>.

If I'm right the markdown code generates correct markdown html and the quoting happens somewhere else .. wild guess: in b-alert?

@bernt-matthias
Copy link
Contributor Author

Note that I have not yet commited the changes to produce markdown in the error reporter python code (which I triple checked to be active) . But tested it with the current client code.

@davelopez
Copy link
Contributor

Ahhh you're right! The renderMarkdown result is what should be in the v-html property, sorry about that.

@bernt-matthias
fixes to gitlab error reporting plugin
77fe9e8 
- URLs for low level gitlab functions (self.gitlab.http_post) need to be
  quoted
- closed issues need to be cached if a comment should be added to existing (potentially closed) issues
@bernt-matthias
do not quote messages from error reporters
f97c6a7 
so far users have seen e.g. `<p>Your error report has been sent</p>`
from the email reporter.
Another example is the link to the created issue of the gitlab error
reporter.
@mvdbeek mvdbeek changed the title [23.0] fixes for (gitlab) error reporting [23.0] Fixes for (gitlab) error reporting Jul 25, 2023
@bernt-matthias @nsoranzo
Update lib/galaxy/tools/error_reports/plugins/github.py
9d2b386 
Co-authored-by: Nicola Soranzo <nicola.soranzo@gmail.com>
mvdbeek
mvdbeek approved these changes Jul 26, 2023
View reviewed changes
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @bernt-matthias!

@mvdbeek mvdbeek merged commit a12ca8f into galaxyproject:release_23.0 Jul 26, 2023
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nsoranzo nsoranzo nsoranzo left review comments

@mvdbeek mvdbeek mvdbeek approved these changes

Assignees
No one assigned
Labels
area/tool-framework area/UI-UX kind/bug
Projects
None yet
Milestone
23.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bernt-matthias @hexylena @davelopez @nsoranzo @mvdbeek