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

✨ Load a wiki page after Selenium is initialized to remind the user to look back at the terminal to answer more questions #242

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

coderanger
Copy link
Contributor

@coderanger coderanger commented Jun 21, 2024

Description

I set the page to Desktop-Tool-Landing but it can be whatever. Some example content (because wikis don't accept PRs):

# Congratulations!

MPCAutofill has started successfully, please look back at your terminal window to answer a few more questions before the upload can begin.

Check out [the documentation if you have any further questions](Desktop-Tool).

Checklist

  • I have installed pre-commit and installed the hooks with pre-commit install before creating any commits.
  • I have updated any related tests for code I modified or added new tests where appropriate.
  • I have manually tested my changes as follows:
    • Ran autofill.py and observed it working.
  • I have updated any relevant documentation or created new documentation where appropriate.

…o look back at the terminal to answer more questions.
@ndepaola
Copy link
Collaborator

thanks for getting the ball rolling on this! i've been meaning to make this change for a few weeks but didn't get around to it.

after some more thought, i think it would be better to load a local HTML page rather than loading a page hosted in the wiki - a) because it means the page should load instantly, and b) because it doesn't really belong in the wiki.

would you mind taking a look at whether it's possible to point selenium at a local HTML file? you may also need to update the pyinstaller build to include the new file

@ndepaola ndepaola added the documentation Improvements or additions to documentation label Jun 21, 2024
@coderanger
Copy link
Contributor Author

Surely, if you want to go even further it could be a localhost webserver and move the config from terminal into a UI? That might be too much though :)

@coderanger
Copy link
Contributor Author

I split the difference, it's setup on an internal web server but just a single static page for now.

@coderanger
Copy link
Contributor Author

Is the test failure real? Seems unrelated in the PDF functionality and those tests haven't been run in a while. I can look tomorrow or something.

@ndepaola
Copy link
Collaborator

ndepaola commented Jun 23, 2024

Surely, if you want to go even further it could be a localhost webserver and move the config from terminal into a UI? That might be too much though :)

we certainly could do yea - though i think it's out of scope here. let's roll with the local webserver approach you have for now and come back to reworking the UI later

Is the test failure real? Seems unrelated in the PDF functionality and those tests haven't been run in a while. I can look tomorrow or something.

it looks like this is because you haven't set up your repo fork with google drive credentials. this is touched on here https://github.com/chilli-axe/mpc-autofill/wiki#contributing but it looks like the docs are a bit out of date - this now matters when modifying desktop tool code as well as backed code. i'll update the docs in a sec to clarify this

the tests failing isn't a big deal at all, they'll rerun with my google drive credentials after i merge the PR anyway

Copy link
Collaborator

@ndepaola ndepaola left a comment

Choose a reason for hiding this comment

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

lgtm pending my one comment, thanks for this PR!

Path(__file__).joinpath("../..").joinpath(constants.POST_LAUNCH_HTML_FILENAME).resolve().read_bytes()
)

def log_request(self, code: int | str = "-", size: int | str = "-") -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

to maintain compatibility with windows 7 for as long as possible, we're still on python 3.9 - so you'll need to use old-style typing here:

Suggested change
def log_request(self, code: int | str = "-", size: int | str = "-") -> None:
def log_request(self, code: Union[int, str] = "-", size: Union[int, str] = "-") -> None:

@ndepaola
Copy link
Collaborator

merging w/ unresolved comment, i'll fix this bit up

@ndepaola ndepaola merged commit e4e820a into chilli-axe:master Jun 24, 2024
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Development

Successfully merging this pull request may close these issues.

2 participants