zach-wahrer / mpv Goto Github PK

Right now, whenever a someone uses the app, their entire tick list is imported into the database. If you are a return user, the app drops your current table, then builds a new one. In the interests of reducing the wait time for users with large tick lists (our most consistent users, I'd guess), I think running a check for each tick, and only inserting new ones, might prove to be faster, at least marginally. I'm going to create a branch to work on this and see.

The only major side effect I can imagine is if you are an existing MPV user, and you delete a tick on Mountain Project, there is no way for the app to know, and therefore it will still be counted in your MPV stats. I think this is a minor trade off that will likely go unnoticed.

Containerize the development instance of MYSQL.

This will simplify setting up and running the application database locally. By adding a simple docker compose file, we can pull the database image from docker and when setting up a development environment. This will help do a few things:

• Ensure all devs are using the same version of MYSQL
• Lay the foundation for adding config/settings files to the Flask application because we have explicitly defined what the development database looks like.

This feature will have no breaking changes and should have bare minimum impact on the existing infrastructure. The only thing that will need to be updated is the README.

For the time being this is only aimed at being implemented in a development environment, this will not change the UI/front-end.

@zachtheclimber Docker is awesome although a bit intimidating if you are new to it. But here is an example of how simple this makes things. So assuming you have docker installed on your machine here is how this would work:

This is a simple docker-compse.yaml file example which would get added to the root project directory of MPV:

version: '3.3'
services:
  db:
    image: mysql:5.7
    restart: always
    environment:
      # Use root/password as user/password credentials
      MYSQL_DATABASE: 'mpv_db'
      MYSQL_ROOT_PASSWORD: 'password'
    ports:
      # <Port exposed> : < MySQL Port running inside container>
      - '3306:3306'
    expose:
      # Opens port 3306 on the container
      - '3306'
      # Where our data will be persisted
    volumes:
      - mpv_db:/var/lib/mysql
# Names our volume
volumes:
  mpv_db:

1. Navigate to the root directory for MPV
2. Run the shell command docker-compose up
   This will download and build a docker image of a MYSQL database using the specified version and environment variables specified in the docker-compose.yaml file.
3. If you want to connect from the shell to the running database image run the command mysql -u root -p mpv_db -h 127.0.0.1 -P 3306
   Notice the port number 3306, inside of docker-compose.yaml we expose this port on the docker container and that will allow us to connect to the containerized instance of MYSQL via that port.

Let me know what you think! Here are a few links to some good reading about docker:

https://hub.docker.com/_/mysql/
https://severalnines.com/blog/mysql-docker-building-container-image

While updating the README, I noticed that point 5 under installing may be obsolete:

5. To run the application on osx or Linux, set the FLASK_APP environment variable to application by running export FLASK_APP=application

With the refactoring of the app layout, I don't think this is required. In fact, when I try it on my Linux machine, it errors out. Just wanted to check with you, @benjpalmer, before I removed it, since I'm not familiar with how Flask functions in OSX. I think it is the same?

Don Ferris III suggested (via Mountain Project):

What if instead of hard coding a value for the route heights you just averaged the existing height values per given style then substitute the blank values with that.  Multipitch would be that averaged value X number of pitches. Perhaps you could even pull the height from nearby routes but I'm betting that would be a bit ridiculous.  

In my deployment of MPV, I use Nginx to proxy requests into a MPV Docker container. When deployed this way, the "Try It!" button works, but entering an email and clicking "Visualize!" only reloads the same page. Headers show that I'm being redirected with a code 307, but no matter how many times I try, or what input I try, I get nothing back. Even invalid email addresses do nothing.

Everything works as it should in a single container, non-proxied environment, but I don't think this would be a standard use case. So I think we need to implement a fix.

I can't tell if this is related to a CSRF token validation issue, or something related to how the Nginx server handles return redirect("/data", code=307) from the main app when data is submitted to /.

Hey @zachtheclimber,

My name is Ben and I am a fellow climber/developer, I had expressed interest in contributing on mountainproject and I always thought it would be fun to work on a climbing related project!!

I wanted to submit this parent issue and get your thoughts on a few ideas below. I think if we address some of the low hanging fruit and other small changes now, it will lay the foundation for adding additional features down the road. If you approve and want to implement anything, we could separate those issues into their own tickets.

I am proposing the following:

• Add contributing guidelines
  If others want to jump in and contribute lets make sure we are all working as a team and know what is expected of one another.
  https://help.github.com/en/github/building-a-strong-community/setting-guidelines-for-repository-contributors
  Issue: #5

• Use a code formatting tool such as Black
  I have used black on other projects and it is awesome. There are other code formatting tools out there as well.
  https://github.com/psf/black

• Unit Testing
  Add unit tests using either unittest or pytest libraries.
  Keep track of code coverage.

• Update app environment config settings
  I think the use of "on" vs "off" in config.json could be replaced with boolean values.
  Maybe some additional configuration optimizations
  https://flask.palletsprojects.com/en/1.1.x/config/

• Overall 'pythonic' refactoring
  Small syntax changes throughout could be improved upon to be more 'pythonic'. I think through unit testing many of these will show themselves. For example:

    # Remove unneeded data fields
    # Delete in reverse order to make field posistions simpler
    remove = [12, 8, 7, 6, 4, 3, 2]
    for row in ticklist:
        for i in remove:
            del row[i]

could be refactored into its own helper function or instead use list comprehension or a generator expression.
clean_ticklist = [[v for i,v in enumerate(row) if i not in remove] for row in ticklist]

• Add type hinting and function annotation
  For example:

def get_userid(email):
    """
    Get username/id using email via MountainProject.com API.
    (https://www.mountainproject.com/data)
    """
    # do some stuff
    return {"status": 0, "name": data['name'], "id": data['id']}

could be refactored to add hinting and annotation such as:

def get_userid(email: str) -> dict:
    """
    Get username/id using email via MountainProject.com API.
    (https://www.mountainproject.com/data)
    """
    # do some stuff
    return {"status": 0, "name": data['name'], "id": data['id']}

Thanks again for open sourcing your idea! This would be my first Flask project as a majority of my experience has been in a Django environment.

I am proposing that we add test coverage for this piece code found in /app/__init__.py:

# Input validation - Thanks to emailregex.com for the regex
regex = r"(^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+\.[a-zA-Z0-9-.]+$)"
if not re.match(regex, email):
    e = "Please enter a valid email."
    return render_template("error.html", data=e)

This could maybe be extracted into it's own helper function. Tests should validate and invalidate many different types of email address patterns.

@zachtheclimber I think this would be a good way to dip your toes into writing some tests, would you want to take this on?

After completing some refactoring of the helper functions in issue #19 , I thought there was a good opportunity to clearly define how we handle application errors. The expected responses can live in one location, and provide an organized and consistent structure throughout MPV. Then, when we add features such as grade conversions and other suggested enhancements, we already have the error handlers registered, defined and ready to be re-used.

Documentation and examples can be found here:
https://flask.palletsprojects.com/en/1.1.x/errorhandling/

@zachtheclimber, do you mind if I take this on?

While the MySQL requirement of MPV can be created using Docker, it would be handy to have the entire project in Docker form. I'm going to start working on that. :-)

While working on the slow CSV download, I discovered another area where we could possibly gain major performance improvements: parse_tick_list(). As you can see from the output below, it is by far the slowest major function at the moment:

_mp_generic_request took 0.4886825085 seconds
fetch_user took 0.4887564182 seconds
parse_user_data took 0.0031590462 seconds
_mp_generic_request took 0.6460049152 seconds
fetch_tick_list took 0.6460957527 seconds
parse_tick_list took 4.7300853729 seconds
db_connect took 0.0144858360 seconds
get_pairs took 0.0021224022 seconds
db_close took 0.0043175220 seconds
db_load took 0.7299504280 seconds
db_connect took 0.0448451042 seconds
height_climbed took 0.0845954418 seconds
pitches_climbed took 0.1354043484 seconds
grade_scatter took 0.0858302116 seconds
grade_scatter took 0.1109952927 seconds
grade_scatter took 0.1010136604 seconds
grade_scatter took 0.0875008106 seconds
grade_scatter took 0.0496525764 seconds
grade_scatter took 0.0368475914 seconds
grade_scatter took 0.0799241066 seconds
grade_scatter took 0.0134923458 seconds
db_close took 0.0014121532 seconds

It would be worth digging into this further to see if we can improve the run time.

I am proposing that we add test coverage for app/helpers. By adding test coverage we can be confident that when MPV is using these helper functions, the application will perform as expected.

What do you think @zachtheclimber? Do you mind if I take this on? Do you have any preference to code structure/styling in regards to one large file vs many smaller files?

I'm wondering if it would help to logically separate some of the helper functions into their own files? For instance an example file could be /helpers/db_connections.py and that file could contain db_connect() and db_close.

The reason I ask, is I am suspecting that by writing tests for some of these functions, there might be opportunity for some light refactoring, or abstracting logic out of some of the large helper functions into their own function or class.

Anyway, interested in hearing your thoughts! Happy to help in any way I can!

I think having the option to log errors to file would be helpful, especially for once (when?) MPV reaches a more deployable stage of development. I know I would like to have it on my production server, so I think I'll work on adding it. @benjpalmer, since you already added logging.exception(), I don't think this will be too hard and should be within my ability level. I'll also try to implement testing for it. :-)

In fixing issue #35, I broke two of the tests in test_errors.py:
TestErrorHandlers.test_mountain_project_api_exception TestErrorHandlers.test_request_exception

Given how I refactored the main app file, this introduced breaking changes with how these tests perform. I tried to fix them, but ran into issues.

MPAPIException and RequestException themselves still work, as evidenced by manual testing, so I think it really is just the tests themselves that need fixed.

I believe this is related to line 47 of the main app file: elif form.validate(): Since the test isn't passing in validated form data, the /data route is just returning index.html, hence the 200 OK code in the output below.

@benjpalmer, perhaps you could make the required tweaks, if you have time? I'll try if you can't. I tried a few fixes that seemed close to resolving the issue, but still couldn't get them to pass. I probably just need to dig in more or refactor a larger chunk of the test.

Output from the relevant tests:

______________________________________________________________________________ TestErrorHandlers.test_mountain_project_api_exception ______________________________________________________________________________

self = <app.tests.test_errors.TestErrorHandlers object at 0x7f5332e0b198>, app = <Flask 'app'>

    def test_mountain_project_api_exception(self, app: pytest.fixture) -> None:
        """Confirms the correct response status and safe error message of MPAPIException."""
        monkeypatch = MonkeyPatch()
        monkeypatch.setattr(requests, "get", self.mock_get_response)
        client = app.test_client()
        app.config["MPV_DEV"] = False  # dev_env to False so a mocked request will be made.
        response = client.post("/data")
    
>       assert response.status == "403 FORBIDDEN"
E       AssertionError: assert '200 OK' == '403 FORBIDDEN'
E         - 200 OK
E         + 403 FORBIDDEN

app/tests/test_errors.py:49: AssertionError
____________________________________________________________________________________ TestErrorHandlers.test_request_exception _____________________________________________________________________________________

self = <app.tests.test_errors.TestErrorHandlers object at 0x7f5332dbb7b8>, app = <Flask 'app'>

    def test_request_exception(self, app: pytest.fixture) -> None:
        """Confirms the correct response status and safe error message of RequestException."""
        monkeypatch = MonkeyPatch()
        monkeypatch.setattr(requests, "get", self.raise_http_error)
        client = app.test_client()
        app.config["MPV_DEV"] = False  # dev_env to False so a mocked request will be made.
        response = client.post("/data")
    
>       assert response.status == "400 BAD REQUEST"
E       AssertionError: assert '200 OK' == '400 BAD REQUEST'
E         - 200 OK
E         + 400 BAD REQUEST

app/tests/test_errors.py:60: AssertionError

When hitting the /data endpoint, The HTML template is looking for :

<script src="static/scripts.js">

I believe somewhere along the way that script file name was renamed to:
scripts.1.js

This is an easy fix, but will resolve the 404 errors being generated by that HTML template.

I know some of the code base is a bit clunky and/or less than optimal. If I see something that is obvious to me, I'll try to refactor it. @benjpalmer, feel free to do the same, since you've noticed some already. If we can, let's try not to make it too complicated/hard to read. :-)

Add a requirements.txt file to keep track of and manage repository dependencies.

Github has a great resource that includes links to examples of other contributing guidelines.
https://help.github.com/en/github/building-a-strong-community/setting-guidelines-for-repository-contributors

Lets open a discussion about things that MPV should include. Here are some things I think we should include and expand upon when defining.

• All code that is submitted should have appropriate test coverage

• Make sure code style guidelines are met.

• Steps to report bugs.

• Steps to request enhancements and other features.

Lets give this some more thought, I am curios to hear other suggestions!

As stated in the title, the loading indicator (spinning white dot and blue dots) no longer functions when you click visualize on the landing page. It does work when you use the test button, however. @benjpalmer, I think this must have broken when you updated the form. Since I wrote the indicator code, I'll dig into it. Just wanted to post a bug report so I'd remember! :-)

As indicated by the bugs found in the beta release, having a test suite for MPV is critical for moving forward. I am currently learning about Python unit testing, so hopefully this will be something I can work on in the near future. @benjpalmer, I know you have a vision for how this could work, so I'm happy to help you in any way I can. :-)

Based on some rudimentary function speed testing, downloading the CSV file from Mountain Project is by far the slowest part of MPV, clocking in at over 6 seconds for a 2000+ entry tick list. With database operations sped up, I would like to take care of this last remaining speed bump.

I spent some time digging into downloading a file concurrently, with the hopes of speeding up how soon MPV has access to the data, but didn't have much luck. I suppose it is possible we are just limited by how quickly the CSV data comes over the network, but I'm not ready to give up just yet! :-)

I did none of this with my original code. I'll go through and add it as I have time, since it isn't critical at the moment.

To make this app more usable internationally, adding a grade conversion option would be helpful. I envision it as a set of drop-downs the user selects on the index page, much like how they can select the unit of measure for distance. It might also make sense to put it in an Advanced drop down area, so as not to clutter the interface initially. Since most of our users are likely to be from the US or use YDS/V-Scale, we can leave that selected by default.

Initially, I think we should add Font grades for bouldering and French for sport/trad. UIAA grades for roped climbing would probably also be easy to add at that time.

In the future, we should add British grades. It could prove to be slightly harder, since it is a dual scale and will likely require special treatment.

If there is demand, we could also add Ewbank grades.

@benjpalmer recommendation:

I think the use of "on" vs "off" in config.json could be replaced with boolean values.
Maybe some additional configuration optimizations
https://flask.palletsprojects.com/en/1.1.x/config/

Nkane 1 suggested (via Mountain Project):

Cool! I'd love to see Max Redpoint (including onsite, flash, redpoint) for trad and sport. That might show progression better (or at least differently) than the mean or mode. Or maybe top 5%.

Adding a feature that allows someone to input their friend's MP email so that their stats could be compared/overlaid would be a neat addition. Might require a different type of output/layout, but should be able to share similar features to what is currently implemented.