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

Added a download button to GUI in order to download reports easily #79

Open
wants to merge 4 commits into
base: old_master
Choose a base branch
from

Conversation

marvindoering
Copy link

Hey,
the GUI is awesome; but is missing one key part. A download button which allows users to download the report more easily.

New pull request because these topics had to be fixed:

  • Fixed issue with download button not working.
  • Set button to only be active once the document has been validated.
  • Fixed issue where the report was simple text and not readable html.

Greetings
Marvin

- Set button to only be active once the document has been validated.
- Fixed issue where the report was simple text and not readable html.
Copy link
Collaborator

@apenski apenski left a comment

Choose a reason for hiding this comment

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

thank you for your PR. I have a few questions and change requests. I am no JS expert though

@@ -57,11 +59,29 @@

fetch('/', requestOptions)
.then(response => response.text())
.then(result => output.innerText = result)
.then(result => {
output.innerHTML = result;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not a javascript expert, but why change from innerText to innerHTML here? Doesn't it work with innerText? I mean with innerText security should be high

Copy link
Author

Choose a reason for hiding this comment

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

Well the problem with innerText is that the result won't actually be displayed on the result page. Its only showing the text which than you will need to put in a html file to actually see the result of the validation. I dont know if this is wanted but for a 0815 user this would be easier to validate their XRechnungen.

Copy link
Author

Choose a reason for hiding this comment

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

What I mean by that is:

With innerHTML you see this as result:
image

With innerText you see this as result:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, then I want definetely innerText! The report isn't html, only can contain html. If you want to display the html parts, we need a different solution and I am not sure whether this is in scope of the validator gui.

Copy link
Author

Choose a reason for hiding this comment

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

Changed back to innerText. Now the report data gets extracted from the div-box like you required it.

@@ -40,10 +40,12 @@
<body>
<script type="text/javascript">

var report;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why storing the report into variable? Isn't better to capture the report from the "result"-element if neccessary?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that can be done but only if the result-element contains actual HTML and not "innerText" otherwise you will be seeing the HTML as text and not as an actual HTML page. And the variable is global which means it can be accessed by other functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to see it as text because it's XML not html

Copy link
Author

Choose a reason for hiding this comment

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

Changed back to innerText. Now the report data gets extracted from the div-box like you required it.

src/main/resources/gui/index.html Outdated Show resolved Hide resolved
.then(result => {
output.innerHTML = result;
report = result;
download.disabled = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be disabled prior request then, or not?

Copy link
Author

Choose a reason for hiding this comment

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

Actually "download.disabled = false" activates the button. The button is disabled at start. See in README.md Line 16

<input type="button" id="dwn-btn" value="Download Report" onclick="return reporting();" disabled>

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had a second submit in mind, with a long processing time. Is your implementation safe then? (not double checked it)

Copy link
Author

Choose a reason for hiding this comment

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

I implemented a check to the javascript that should prevent downloading if there is not data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for not reviewing for long time. I wanted to prepare a release and put this into it. I checked your implementation and noticed just now that only focus on included html. I guess your are - as most of the users - using the validator with xrechnung configuration and then it makes sense to somehow extract the included html fragments.

But including html fragments into the report is a decission xrechnung validator configuration project did. The validator does not know about "embedded" reports/html. And since we have other configurations as well, extracting/downloading only parts of the report wouldn't make sense. Only downloading the complete report would.

src/main/resources/gui/README.md Outdated Show resolved Hide resolved
- Changed back to innerText on output
- replaced global var report and extracted info for download from div
- put another check on the download button to insure that the report can only be downloaded once data is available
rkottmann pushed a commit that referenced this pull request Jun 2, 2021
rkottmann pushed a commit that referenced this pull request Jun 2, 2021
rkottmann pushed a commit that referenced this pull request Jun 2, 2021
rkottmann pushed a commit that referenced this pull request Jun 2, 2021
rkottmann pushed a commit that referenced this pull request Jun 3, 2021
rkottmann pushed a commit that referenced this pull request Jun 7, 2021
rkottmann pushed a commit that referenced this pull request Jun 7, 2021
rkottmann pushed a commit that referenced this pull request Jun 7, 2021
rkottmann pushed a commit that referenced this pull request Jun 7, 2021
rkottmann pushed a commit that referenced this pull request Jun 7, 2021
rkottmann pushed a commit that referenced this pull request Jun 12, 2021
rkottmann pushed a commit that referenced this pull request Jun 29, 2021
rkottmann pushed a commit that referenced this pull request Jun 29, 2021
rkottmann pushed a commit that referenced this pull request Jun 29, 2021
rkottmann pushed a commit that referenced this pull request Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants