ChristopherChudzicki / math3d

Legacy version: Use math3d-react repo instead
MIT License
12 stars 4 forks source link

Add graph storage function and short urls #75

Closed stardust66 closed 7 years ago

stardust66 commented 7 years ago

This PR has lots of commits. It includes all the commits from when I started restructuring for flask. At first there were login and registration features but they got deleted from the UI. I've kept the backend implementation of those features to make it easier to add them back in later. The code is live on http://math3dstaging.herokuapp.com/.

An issue is the page behaves weirdly on mobile. http://math3d.org on mobile starts out with the menu hidden but the staging version has the menu visible, and the arrow to hide the menu somehow ends up on the bottom of the page. When you click the show/hide button everything seems to work again.

@ChristopherChudzicki Please tell me what I can improve.

Edit: I tracked down the mobile problem and fixed it. The issue was caused by my use of ng-cloak. This delays the displaying of certain elements until angular finishes to load because the angular interpolation doesn't kick in quickly enough sometimes causing the page to display content with [[]] tags. The main page doesn't have interpolation so ng-cloak has been removed. It was making calls to hideMnu() not work.

stardust66 commented 7 years ago

Related to #68

ChristopherChudzicki commented 7 years ago

@stardust66 Thanks for this PR, Jason! I'll look at it more closely this weekend and hopefully merge it then.

One thing I'm thinking about is whether title should be part of your Python Graph class, or if should go in the javascript math3d.settings. I think it's probably better to put title in math3d.settings, then it can be handled the same as all the other serialized settings.

And, although I know I mentioned title to you in person today, if we go the route I suggested above, then title wouldn't necessarily need to be part of this PR. (But could be if you want).

What do you think?

stardust66 commented 7 years ago

One problem with having the title only in the serialized settings is that when displaying a list of graphs by a user in the future (assuming that's part of the plans), we have to grab the serialized setting from each graph, decode it, and display the title, instead of having it readily available as part of the graph object sent from the server. What do you think about having a title in both the graph object and the javascript side? Although that doesn't seem very elegant. I think for now I might just remove the title for this PR until we figure out what to do.

ChristopherChudzicki commented 7 years ago

@stardust66 I've been testing this locally and things things have been working great! A few thoughts:

  1. Licenses and Copyright: Please put the MIT license in large new files you contribute like app.py with copyright in your name. For existing files that you've edited like math3d_app.js, add a line to the copyright at top:

    Copyright (c) 2017 Christopher Chudzicki
    Copyright (c) 2017 Jason Chen

    (Or Chen Luming?)

  2. Saved URLs and Query String: Should saveToDB() throw away the query string? right now it does not. One consequence of this is that if you load an 'old-style' (i.e., loooong) saved graph URL, then save it, you don't get a short URL. [Try it with this example.]().

  3. Title: Re title, your concern above is a good point. We discussed this a bit the other day: Maybe best to instantiate Graph class with a metadata dictionary and serialized graph settings. Might have to serialize metadata, too, but that seems like a more extensible plan.

    For the UI, I also think it would be better if title was set outside the modal, and [Share] button generates URL immediately. (With title in modal, you need two clicks, [Share] and [Save] to get a shareable URL.)

Again, this is great, and it will be nice to have shorter URLs at math3d.org! I'm going to send you a separate email with some thoughts about how we might smoothly transition from hosting on Github Pages (where it is now) to hosting on Heroku.

stardust66 commented 7 years ago

@ChristopherChudzicki I looked up how to store dictionaries in Postgres and it seems like the way to go is to make another table called Metadata. Each row would be associated with a single graph, and would contain the title. If we don't make another table then we need to implement serialization of the dictionary when storing in the database. Which approach do you think we should take?

ChristopherChudzicki commented 7 years ago

I don't have a lot of experience with databases — either approach seems reasonable to me. If you go the two – table route both tables would be indexed by the same hash ID, right?

stardust66 commented 7 years ago

Well they would have references to each other. So it's as if each Graph contains a Metadata object.

stardust66 commented 7 years ago

@ChristopherChudzicki Should the author be in the metadata section as well?

Edit: Check out these lines. I think it makes sense for the short_url to be in the metadata as well because it makes the code redundant not to have it there. See my explanation with this commit.

ChristopherChudzicki commented 7 years ago

Sure, we can put author in metadata as well. Thanks for fixing URL generation issues!

stardust66 commented 7 years ago

To locally test the newest changes requires running

$ python manage.py db upgrade

The structure of the database has changed. The newest changes have been pushed to math3dstaging and the database there has been updated. To update the database for math3d run

$ heroku pg:reset --app math3d
$ heroku pg:push math3d DATABASE_URL --app math3d

assuming the local database is called math3d. The pushing part isn't possible under school WiFi for some reason so a phone hotspot is needed.

ChristopherChudzicki commented 7 years ago

@stardust66 I just added a header to math3d.org with contact information. I probably won't work more on this till the weekend, but this makes me more comfortable merging such a large PR as this—now people can tell us if something goes wrong :)

stardust66 commented 7 years ago

That's great! I like the new colors. Conflicts have been resolved by merging in my repo.

ChristopherChudzicki commented 7 years ago

@stardust66 I've been working through this Flask by example tutorial to try and gain more familiarity with Heroku and PostgreSQL. (I had no experience with databases before you started this project, and have only learned a little bit so far.)

One thing I noticed from this tutorial is that PostgreSQL has a JSON data type.

Should we using the json (or jsonb) data type to store the graph settings rather than storing the serialized string? Does it matter? Interested in your perspective.

=============== On a different note: I'd like to merge this in the next day or so, but I'm not quite ready to put it on the main math3d.org page yet. So my plan is:

  1. I've already switched math3d.org to feed from gh-pages branch.
  2. Merge this PR into master. (Won't affect gh-pages immediately)
  3. I'll put this branch on Heroku math3d and math3dstaging
  4. I want to continue making a few tweaks and experimenting with the database. Then in the next week or so we'll switch the custom URL math3d.org to Heroku.

(By the way, I upgraded math3d Heroku to a hobby dev plan, so the dyno won't sleep.)

ChristopherChudzicki commented 7 years ago

@stardust66 This is finally live. I moved the main URL to heroku today.