Modified builders.py and Add frontend page integration #839
Conversation
|
@Salmathmullali Thank you for your PR. Can you please tell which issue is fixed by your PR? |
|
Hi @kelson42, thank you for your response! This PR is intended to address issue [#825] checking the ZIM file status before redirecting. In my first PR, I received feedback from @audiodude suggesting I move the logic into builder.py (around line 227) and also include a frontend page that explains the expiry issue and provides a button to re-request the ZIM. I followed that advice and updated the code accordingly. In this current PR: I’ve modified builder.py as per the suggestion. The frontend part is working fine when accessed directly. However, I ran into an issue where the API returns a “Not Found” error when trying to handle expired files. As a beginner, I wasn’t sure if the problem was with my routing or the API itself, so I wanted to submit my progress so far and ask for guidance on how to debug or structure this better. I’d really appreciate any suggestions or pointers. I’m willing to keep improving this PR and learn as I go. Thanks again! |
There was a problem hiding this comment.
This PR looks like it's not only incomplete, but doesn't even partially solve any stated problems. There are multiple significant issues that need to be addressed. Please test your code and make sure that it works fully before submitting another review request.
In particular, we will not be able to merge any code that says:
// TODO: Call API backend
Successful PRs must include complete solutions.
Finally, this PR doesn't include any tests. Full frontend and backend tests for all added/modified changes are necessary for any PRs.
Thanks!
There was a problem hiding this comment.
If you need a newer version of attrs, please put the version number you need in this file. Also no need to change the ordering.
| </script> | ||
|
|
||
| <style scoped> | ||
| .container { |
There was a problem hiding this comment.
We shouldn't need many, if any, component styles. Please use Bootstrap classes for styling.
There was a problem hiding this comment.
Please include a screenshot of this frontend page.
| title: () => BASE_TITLE + ' - ZIM file', | ||
| }, | ||
| }, | ||
| { path: '/', component: IndexPage, meta: { title: () => BASE_TITLE } }, |
There was a problem hiding this comment.
Please restore the previous formatting of the routes array.
| ]; | ||
|
|
||
| const router = new VueRouter({ | ||
| mode: 'history', |
There was a problem hiding this comment.
What does this do and why is it necessary?
|
|
||
| try: | ||
| # The credentials module isn't checked in and may be missing | ||
| from wp1.credentials import ENV, CREDENTIALS | ||
|
|
There was a problem hiding this comment.
Please restore 2-space indentation. You can run pipenv run yapf -i wp1/web/__init__.py to properly format the file. You should also consider installing the pre-commit hooks mentioned in the README, which will make sure that you cannot accidentally commit Python files with incorrect formatting.
| return f(*args, **kwargs) | ||
| flask.abort(401, 'Unauthorized') | ||
|
|
||
| @app.route('/') |
There was a problem hiding this comment.
This looks like debug code and should be removed.
There was a problem hiding this comment.
I'm not sure there are any significant changes to this file, besides changing the formatting and adding a debug function. Consider reverting it.
There was a problem hiding this comment.
It looks like all you did to this file was remove all the comments? Maybe you have a bad commit somewhere?
| methods: { | ||
| async checkZimStatus() { | ||
| try { | ||
| const response = await fetch('<WASABI_STORAGE_URL>', { method: 'HEAD' }); |
There was a problem hiding this comment.
You still should not be doing this in the frontend, and the fact that you're using the literal string <WASABI_STORAGE_URL> demonstrates a lack of understanding of how the fetch method works.
Was this code copied from an LLM by any chance?
|
Sir, Thank you so much for all the guidance and detailed feedback you've provided . I really appreciate the time and effort you put into helping me learn. To be transparent, I'm self-taught and currently learning on my own without a mentor. This is my first time contributing to a real-world project of this scale, and while I’ve been making mistakes, I’m committed to learning from them and improving. Your feedback is helping me understand not just the code, but also how to work within a team and follow best practices. Regarding your question about LLM usage: yes, I’ve used it occasionally . Mainly for guidance on fixing specific errors or improving explanations . But the core understanding and effort are my own. My goal is to grow as a contributor and become a valuable part of this project. I may be new, but I’m putting in hard work to learn and contribute meaningfully. I’ll now focus on addressing your comments in the PR and making the necessary corrections. Thank you again for the opportunity to learn from you. Hopefully |
|
Hi Salmath, First off, no need to address me as sir. 😄 I appreciate your honesty, as well as your enthusiasm to learn and contribute. I hope you find my comments to be helpful, fair, and not (too) judgemental. I'm looking forward to seeing your revisions and improvements to this PR and will gladly review it further once it's ready. |
|
Thanks for the feedback, really appreciate your guidance 🤝 I'll work on the changes and update soon! |
Fixes #825
Sir,
I’ve added the builder functionality and an additional frontend page (for expired zim function). The frontend page works as expected. However, I encountered an issue with the API, where it shows "Not Found" when trying to retrieve files. I tried to debug it but could not resolve the issue, and I am still new to working with APIs. I thought I should submit my changes and leave this part open for now.
I look forward to any feedback or suggestions on how to proceed with fixing the API issue.
Thanks.