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

Download Results as pdf feature was added to the streamlit webapp #82

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ShudarsanRegmi
Copy link

User can now download the generated results as pdf.
image
image
image

@LuluW8071
Copy link
Contributor

LuluW8071 commented Apr 26, 2024

For downloading feature I don't think columns like longitude and latitude are needed👍🏻

Also add note specifying what is scode and cscode at the bottom in pdf

app.py Show resolved Hide resolved
Copy link
Contributor

@dhirajraut1 dhirajraut1 left a comment

Choose a reason for hiding this comment

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

This adds the logo as well. Just an aesthetical change.
BTW really love the work you've done.
image

ShudarsanRegmi and others added 2 commits April 26, 2024 17:48
Co-authored-by: Dhiraj Raut <63958838+dhirajraut1@users.noreply.github.com>
@ShudarsanRegmi
Copy link
Author

For downloading feature I don't think columns like longitude and latitude are needed👍🏻

Also add note specifying what is scode and cscode at the bottom in pdf

Need recommendations on this. Shall I make changes as mentioned above?

@dhirajraut1
Copy link
Contributor

Hey @ShudarsanRegmi, I have a few suggestions:

  1. Allow the user to view the downloaded directly from the UI itself. A simple toast with file URL should do the trick. I'm not sure though.
  2. Store the downloaded file in a folder named something like exports or genearated-pdfs
  3. Reduce the margins in the header to be bare minimum. Currently, there's too much gap between the elements in the heading section.
  4. Only display required columns in PDF as suggested by @LuluW8071
  5. Consider the linting of the code in order to make the code more standard.

@ShudarsanRegmi
Copy link
Author

@dhirajraut1
image
Is it okay now? Do you recommend any further changes?
Shall I try moving the logo to the left or it is fine?

@dhirajraut1
Copy link
Contributor

Looks good to me.
I have made some changes that allows the user to download the files. I'd be more than happy to collaborate.
image

@LuluW8071
Copy link
Contributor

LuluW8071 commented Apr 26, 2024

  • Allow the user to view the downloaded directly from the UI itself. A simple toast with file URL should do the trick. I'm not sure though.

By this do you mean viewing the contents of pdf from streamlit?

@dhirajraut1
Copy link
Contributor

By this do you mean viewing the contents of pdf from streamlit?

@LuluW8071 I meant the user to be able to download the file to their desired file location. Non-technical user don't have to see the file in the source directory. They can simply download the file directly. I have created something similar. See the screenshot below.
image

@ShudarsanRegmi
Copy link
Author

@dhirajraut1 Is this project even for distribution for general users? This is meant to be run by the exam conducting comittee, isn't it? Why to worry too much about the download path ?

Copy link
Contributor

@LuluW8071 LuluW8071 left a comment

Choose a reason for hiding this comment

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

In Windows, if you encounter an error like: OSError: No wkhtmltopdf executable found: 'b', you can resolve it by installing wkhtmltopdf from here.

After installation, navigate to the installation directory and add the bin folder to the environment variable path:

C:\Program Files\wkhtmltopdf\bin\

You may need to restart your system.

Also another issue for windows may be unicode display error(I couldnt find fix for this):

Not necessary for deployed server. This bug is for maintainers on windows platfom
image

@ShudarsanRegmi
Copy link
Author

ShudarsanRegmi commented Apr 26, 2024

User can now View the downloaded file directly in the browser. The rendered pdf can again be downloaded to any desired location.
Further. Irrelevant columns from both the pdfs were removed and the generated files are stored in generated-pdfs folder.
Margin issue on header section was also fixed.
image
image
image

@sapradhan
Copy link
Collaborator

Thank you for your effort but I see few fundamental issues with this feature -

  • There is a manual verification step which cannot be eliminated at this point. Therefore the generated by this app is at best intermediate, does not make sense to have that in print friendly format like pdf
  • See center allocation notice published by NEB it contains centers from all districts and crucially it contains registration number distributions for schools. Script must be able to distribute registration numbers before generated PDF would be useful.

Also note this is owned by NEB and not by MoEST directly, therefore MoEST logos would be inappropriate

@ShudarsanRegmi
Copy link
Author

@sapradhan It may not be ready for final

  • There is a manual verification step which cannot be eliminated at this point. Therefore the generated by this app is at best intermediate, does not make sense to have that in print friendly format like pdf

The script is supposed to run again and again until all the students of all schools have not been assigned a center and if the distribution of centers is uneven. If the script gives tne best of its output then It will have to be reviewed by many other members and there will be manual allocations further. For this purpose alone it may be useful, isn't it ? Instead of sharing files like csv or tsv, better pdf can be distributed among the reviewing members.

@sapradhan
Copy link
Collaborator

Instead of sharing files like csv or tsv, better pdf can be distributed among the reviewing members.

CSV and TSV are more suitable for review, edits as they can be easily dealt with spreadsheet applications. This is also for only one district, merging with other districts will also be necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants