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

New download action #303

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

Conversation

RohitP2005
Copy link

Pull Request Description - #298

Summary of Changes:

  • Added the download_file function to handle file downloads by clicking a specified element (bid).
  • Ensures the download directory exists, monitors progress, and saves the file upon completion.

Reason for Changes:

  • Automates the download process with real-time progress updates and saves the file to a specified directory.

Example:

  • Starting Download:
    Starting download...

  • During Download:

Downloading... Elapsed time: 1 seconds
Downloading... Elapsed time: 2 seconds
  • On Completion:
    Download completed: downloads/my_report.pdf

How to Test:

  1. Call download_file with a valid bid and optional download_path.
  2. Verify the download starts and the file is saved correctly.

@RohitP2005
Copy link
Author

I'm still learning and apologize for any mistakes. I'm open to corrections and feedback. Thank you for your understanding!

@gasse gasse changed the title Issue#298 - Support download action New download action Jan 13, 2025
Copy link
Collaborator

@gasse gasse left a comment

Choose a reason for hiding this comment

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

Thanks @RohitP2005 ! This looks good, could you please add the following:

  • remove calls to print() when executing the action
  • write a unit test to test this

elem = get_elem_by_bid(page, bid, demo_mode != "off")
add_demo_mode_effects(page, elem, bid, demo_mode=demo_mode, move_cursor=True)

print("Starting download...")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is useful for debugging but should be removed once your code is working. Please remove these.

Comment on lines +653 to +658
# Monitor the download progress
start_time = time.time()
while not download.is_done():
time.sleep(1)
elapsed_time = time.time() - start_time
print(f"Downloading... Elapsed time: {int(elapsed_time)} seconds")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. This for loop is not needed in the end, the save_as call will wait for the download to complete.

download.save_as(file_path)

# Notify the user about download completion
print(f"Download completed: {file_path}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be removed.

@gasse
Copy link
Collaborator

gasse commented Jan 13, 2025

@recursix can you have a look as well? This action can potentially overwrite files on the filesystem, do you think we should do something about it? We discussed that with @ThibaultLSDC a while ago, ideally we could provide a safe directory for all file_upload and file_download operations, which would be enforced to prevent the agent reading / writing stuff anywhere on the file system.

@gasse
Copy link
Collaborator

gasse commented Jan 13, 2025

I'm still learning and apologize for any mistakes. I'm open to corrections and feedback. Thank you for your understanding!

No worries at all, we are all making mistakes and still learning :)

@RohitP2005
Copy link
Author

I'm still learning and apologize for any mistakes. I'm open to corrections and feedback. Thank you for your understanding!

No worries at all, we are all making mistakes and still learning :)

Thanks you,

I also think we should have a dedicated directory for this action.

@RohitP2005
Copy link
Author

hey @recursix any updates on the dir of file_download options
or should i just fix the failing CIs

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.

2 participants