jbholden / cdcpool_google

0 stars 1 forks source link

Coding for create_week #10

Closed blreams closed 10 years ago

blreams commented 10 years ago

I've done the initial coding on the create_week handler that is to be used by commissioner to create a new slate of weekly games. The handler uses a combination of:

At this point the handler is fully functional except for actually loading the week into the datastore.

I have a few questions:

  1. One of the checks is to make sure that the week/year combination specified on the form does NOT correspond to a week that already exists in the datastore. Which means a query...is this query available from memcache?
  2. The form uses drop-down list boxes to select teams for each game. Right now I query the datastore to get the list of available teams. Is this query available from memcache?
  3. Once I add the code to submit the weekly slate to the datastore, I assume the current test for week will still function correctly.

Also, I would like for you to play with the form...make sure it works as expected (it's pretty tedious to fill in everything). You can fill in as much or as little as you like. Until the form is completed correctly, it will keep re-displaying the get form with entered data intact and appropriate error messages.

jbholden commented 10 years ago

Question 1:

Yes, there is a memcache value that has the weeks and years. See the file: code/database.py. You can use load_weeks_and_years, get_years, or get_week_numbers. I also have that code that checks for a missing year or week value (checks parameters passed into the week_results page) so it might make sense to create a function like is_week_valid(year,week_number) in database.py.

load_weeks_and_years returns a dictionary. The key value is the year number, the value is a list of all the valid week numbers for that year.

from code.database import *
d = Database()
weeks_in_year = d.load_weeks_and_years()
missing_year = 2013 not in weeks_in_year
missing_week = 1 not in weeks_in_year[2013]
jbholden commented 10 years ago

Question 2

Yes, there is a function to get all the team names. See the file: code/database.py. There is a function called load_teams(). This function returns a dictionary. The key value is the string key value of the team object in the datastore. The value is the team datastore object.

I used the datastore key value as the dictionary key to provide a fast lookup of the team name from a Game object. The Game.team1 or Game.team2 value can be used as the key value to lookup the team object.

from code.database import *
d = Database()
team_dictionary = d.load_teams()
team_objects = team_dictionary.values()
team_names = [ team.name for team in team_objects ]
jbholden commented 10 years ago

Question 3

It appears that the most of the current test code will basically ignore this new week. So the existing tests should pass, but we might want to add in the functionality to test the new week.

The only exception is the test ('test the week results page',TestWeekResults). This test will try and fetch the week results web page for each week in the database and make sure it is fetched successfully. This test is not in the master branch yet, but you can find it in the "brent_develop" branch in the /tests/test_week_results.py file.

Existing Tests That Won't test the new week

The problem is that the week numbers to test are hardcoded. I did this before adding functions to database.py to fetch the years and week numbers in the database. These tests should be changed to use these functions to test all the current years and weeks in the database.

jbholden commented 10 years ago

One other note: When you create a new week you need to ensure that the memcache gets updated with the new data. There might be some code in /code/database.py and /code/update.py that will help you do this easily.

blreams commented 10 years ago

I implemented is_week_valid() in database.py (you will see that in my next push). I'm also getting a sorted list of teams from load_teams() as you suggested.

I'm starting to work on the code that loads the datastore. This will be done in two pieces. The first piece will create an instance of LoadGames and use the add_game() method to add all 10 games to the Game datastore. The second piece will create an instance of LoadWeeks and use the add_week() method to add the week (with the appropriate list of recno's corresponding to the games) to the Week datastore.

The part I'm confused about is recno. Entities in Game and Week each have a recno, but it doesn't appear that recno is unique. IOW, recno 1 exists in both datastores, but within a particular datastore, recno must be unique. What I don't understand is how to get a unique recno for each entity. I suppose I could to a memcache lookup and get all Game entities, count them, and increment...likewise for Week entities. Or do you have a better idea?

jbholden commented 10 years ago

Sorry for the confusion. You shouldn't use any of the code in the /load directory. This is only for loading the database with the existing data (and is a bit of a hack). This is my 3rd implementation of the cdcpool. The first implementation used apache and a version of the oracle berkley database. The 2nd version was using node.js. The 3rd version is this project.

The recno field is left over from the 1st implementation. Instead of re-writing the code that reads the excel spreadsheet, I used the oracle database data to generate the /load/* files which loads the data in the google datastore.

You will need to write code to create the datastore objects (initialize the model values and then call put()) and also to assign the keys that some of the property references to.

jbholden commented 10 years ago

Here is some pseudo code for creating a week that might help.

for each game:
   create a game datastore object
   lookup the team object key value for team1 and team2
   assign the team1 and team2 key
   put in datastore (save key value)

create a week datastore object and assign values
assign week.winner = None  (no winner yet)
assign weeks.games to a list of game keys (from above)
put week in datastore

Looking up the team key value based on the team name is kind of a reverse mapping of the current team dictionary returned from database.py load_teams().

For the 2nd method (create new data structure), you could do something like a dictionary where the key is the team name and the value is the key value. You could store this structure in the memcache and then quickly get the team names (for the create week page team names by calling the .keys() method) and also quickly lookup the key values. This could be added to the existing load_teams function or you could add a new one (probably in the same file database.py).

jbholden commented 10 years ago

Some Feedback on the Create Week Page

body {
   color : black;
   font-family: Helvetica;
   font-weight: 100;
   font-size: 10pt;
}

Human Computer Interaction

blreams commented 10 years ago

Thanks, I like all of your suggestions. I didn't spend much time on the "style" of the page because I figured that would happen later. My goal was to get the bones of the page working. Style is something we should address holistically, and that can happen near the end. BTW, I'm generally not a fan of table borders unless the table is in fact representing tabular data. I turned them on here mainly because I wanted to see the grouping of form elements but they will go away or be softened eventually.

Let's open an issue placeholder for "site style" and revisit later.

I couldn't decide if I wanted to offer a way to edit existing weeks. I do think it makes sense and it wouldn't be much additional work with the existing html template and code. However, there needs to be a clear demarcation where pick sheet edits are not allowed. For example:

Here is where I struggle with modifying the pick sheet after poolers have submitted picks:

In the past, we have had cases where a commissioner has created a faulty pick sheet (usually a bad spread) and we didn't figure it out until some pooler inquired about it.

jbholden commented 10 years ago

Summary of the possibilities

  1. enforce that no changes can be made once one pooler has submitted a pick
  2. allow a change but force all poolers to repick all picks
  3. allow a change and have poolers only repick the affected games
  4. throw out the games that have an error
  5. any others?

List of possible errors

Here is what I think is the easiest implementation

Issues with the repick possibilities (numbers 2,3)

I think number 3 is the best case, but difficult to implement

jbholden commented 10 years ago

One other note, there is a possible race condition in this case:

allow changes until a pooler has submitted a pick if the spread is incorrect, otherwise live with the error

blreams commented 10 years ago

Race conditions...ugh. More thinking out loud.

When a pick sheet is created/modified, we could store the timestamp, or better yet create a revision number that increments each time the pick sheet is modified. When a pooler submits their picks, we capture the revision number of the pick sheet that they used. That would be a relatively simple check to perform when a pooler submits their picks...just make sure their pick sheet revision agrees with the current pick sheet revision.

Bad spreads are relatively benign. Since everyone uses the same spread, you could argue that everyone is in the same boat. Although astute poolers could pick up on the fact that the spread is wrong and quietly use that to their advantage.

Wrong teams being entered...I think you just have to throw that game out (either everyone gets it right, or everyone gets it wrong).

The bad year or week value can be remedied with a check applied to the creation of the pick sheet. Here's an idea, I've already implemented code that pre-selects the week and year based on the current database:

blreams commented 10 years ago

After thinking some more about the revision number idea when creating/modifying pick sheets...it would probably be easier overall just to use a timestamp. That way you can just blindly write the timestamp when you do the put to the database. With revision incrementing, you would have to first read the entry to get the current revision, then increment it before the put.

jbholden commented 10 years ago

I would recommend adding the following to the game datastore model (/models/games.py). This will automatically fill in the creation and modified times. (similar to what is in /models/picks.py)

    created = db.DateTimeProperty(auto_now_add=True)
    modified = db.DateTimeProperty(auto_now=True)

Also, for the create games form, can you add code to input the game start time as well (goes in the Game.date field)? I found it nice to see the game start time when viewing a player's results. Also, please take into account the timezone. It would be nice for someone on the west coast to see the time in their timezone.

jbholden commented 10 years ago

Here is some help on the timezone code. I added the file /utils/timezone.py (currently in the brent_develop branch on github https://github.com/jbholden/cdcpool_google/blob/brent_develop/utils/timezone.py).

Basically it works like this:

import utils.timezone as tz

# create a date using the timezone, set the tzinfo= field to the timezone
testdate = datetime.datetime(2010,2,24,19,30,tzinfo=tz.Eastern)

# available timezone values
tz.utc
tz.Eastern
tz.Central
tz.Mountain
tz.Pacific

# get the time in the desired timezone
utc_time = testdate.astimezone(tz.utc)
eastern_time = testdate.astimezone(tz.Eastern)
central_time = testdate.astimezone(tz.Central)
mountain_time = testdate.astimezone(tz.Mountain)
pacific_time = testdate.astimezone(tz.Pacific)

Also, I could not get the local time zone working, so I was thinking that as part of the pooler's user data they should also specify what timezone they prefer.

blreams commented 10 years ago

I added the created and modified fields to the Game table in models/game.py. I also added form selectors on the create_week page to allow the commissioner to enter kickoff datetime. I used a HTML5 input type called datetime-local. It doesn't work nicely in all browsers, but in chrome it gives a nice calendar date and scrolling time selector. Not supported in Firefox so it just shows a regular input text box. At this point I don't want to put a lot of effort into being browser agnostic nor did I want to spend cycles on allowing free-form date/time input. The error string shows what format is expected for the datetime string if it doesn't like what it sees. I can pretty that up later.

Also, for now, I assume tzinfo=tz.Eastern. Once we get timezone info into the User table, I can fix that. We shouldn't limit timezones to the US either (a couple of our players are in the UK).

jbholden commented 10 years ago

Do you think we should try and use the pytz module? I think this would allow you specify any timezone by a string value. I believe you just store the time in UTC format (this is how google datetimeproperty is stored) and then convert to a different timezone when displaying it to the user.

I think it might work something like this.

from pytz import timezone
import pytz

date_in_utc = ...retrieve from datastore

amsterdam = timezone('Europe/Amsterdam')
date_in_amsterdam = date_in_utc.astimezone(amsterdam)

A few notes and issues with this method

blreams commented 10 years ago

Thanks for the pointer. I was able to get gae_pytz working. In my byron_develop branch I added the folder 'pytz'. In my code I added: from pytz.gae import pytz

In my code (temporary until we get TZ preferences for poolers), I did the following: mytz = pytz.timezone('US/Eastern')

Any place where I create a datetime object, I localize it as follows: utc_dt = mytz.localize(datetime(year,month,day,hour,minute)) This creates a datetime object that is the UTC equivalent of the specified time from whatever TZ mytz is.

We just have to make sure that anyplace where we display date/times, we do so in the preferred TZ as follows: local_dt = utc_dt.astimezone(mytz)

I haven't pushed this from my local byron_develop branch to the remote byron_develop branch on github yet. Will do so soon.

jbholden commented 10 years ago

I tried to get the same thing running in my branch (before I saw your message) and had a little trouble, but was able to get it working. A few notes:

from pytz.gae import pytz
mytz = pytz.timezone('US/Eastern')   # your implementation
mytz = pytz.pytz.timezone('US/Eastern')   # my implementation
blreams commented 10 years ago

I think it is because of the way you imported pytz. In my code I used from pytz.gae import pytz. You must have used import pytz.gae as pytz.

I definitely like the idea of having utility functions available in utils. Come up with something and I'll start using it.

The preferred timezone (or mytz object) would need to be stored in the Player table, wouldn't it?

blreams commented 10 years ago

I just committed my development branch (byron_develop) and merged this with master. I'm closing this issue because the bones of the code is done and functional. I will open a separate issue to track edit_week development.