Skip to content
This repository was archived by the owner on Feb 5, 2018. It is now read-only.

add hotel room update timestamps to CSV export - #71

Open
jcooter wants to merge 3 commits into
masterfrom
jcooter-hotel-changes
Open

add hotel room update timestamps to CSV export#71
jcooter wants to merge 3 commits into
masterfrom
jcooter-hotel-changes

Conversation

@jcooter

@jcooter jcooter commented Dec 17, 2016

Copy link
Copy Markdown

This change modified the Room model to add an update timestamp, and modifies the gaylord CSV export to include the update timestamp in the CustomField2 column.

@jcooter
jcooter requested a review from kitsuta December 17, 2016 21:27
@EliAndrewC

Copy link
Copy Markdown
Contributor

Question for @bds002: this change will make it so that going forward the last updated timestamp will be displayed in that column. However, when this is deployed, the "last updated" timestamp will be initially the same for all rows, since for existing columns we're just setting it to the current timestamp and then keeping it up to date moving forward. Is this okay? Or do we need to make sure that all of the existing "last updated" timestamps are correct for all of our existing records?

@bds002

bds002 commented Dec 18, 2016

Copy link
Copy Markdown
Contributor

As of this moment. Current time is fine. Everything has been sent to Hotel team for current time.

@EliAndrewC

Copy link
Copy Markdown
Contributor

+1 from me in that case, though I'm mostly just assuming the SQLAlchemy stuff is in line with how we're doing it elsewhere, since that's not something I've taken the time to look into yet.

@binary1230

binary1230 commented Dec 19, 2016 via email

Copy link
Copy Markdown
Contributor

binary1230
binary1230 previously approved these changes Dec 27, 2016

@binary1230 binary1230 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 from me EXCEPT you may need to move the alembic stuff from here to the magprime repo (we have done the alembic stuff as per-event [i.e. magprime, magstock, etc])

@kitsuta is the alembic expert so I would get her opinion on the placement

@binary1230 binary1230 changed the title Implementing #2212 add hotel room update timestamps to CSV export Dec 27, 2016
@binary1230

Copy link
Copy Markdown
Contributor

fixes magfest/ubersystem#2212

@kitsuta kitsuta left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oops, when we talked about this in Slack I totally didn't realize that the migration was in the hotel plugin.

DB migrations are 'per event' because they're generally linear (alembic allows non-linear but it's a pain). So we do want to keep migrations out of stuff like the 'hotel' plugin.

I'll copy the relevant stuff to our magprime plugin but all the alembic stuff should be removed in this branch before merging.

kitsuta added a commit to magfest/magprime that referenced this pull request Dec 28, 2016
@binary1230
binary1230 dismissed their stale review December 28, 2016 22:44

kitsuta is right

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants