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

Use a query to update public_on #1027

Closed
wants to merge 1 commit into from

Conversation

pascalj
Copy link
Contributor

@pascalj pascalj commented May 25, 2016

Because #1024 introduced a bug (Alchemy::Page.each is not defined) I took the occasion to rewrite the migration so it does not rely on classes any more. This makes it independent from the implementation and faster.

I could only test it with Postgres, but it should work with MySQL also. I will try in the next few days when I have time. Feedback is appreciated!

@TeatroIO
Copy link

I've prepared a stage to preview changes. Open stage or view logs.

@mamhoff
Copy link
Contributor

mamhoff commented May 25, 2016

Nice! One question though: What timezone is NOW() in? The timestamps are in UTC, so we need to make sure that NOW() does the right thing.

@tvdeyen
Copy link
Member

tvdeyen commented May 25, 2016

👍 thanks

@tvdeyen
Copy link
Member

tvdeyen commented May 25, 2016

@pascalj since Rails stores timestamps as UTC and, as far as I know, NOW() uses the server timezone we need to convert the NOW() selects into UTC. As this is different for each database we probably should use Rails to get the correct value.

@tvdeyen
Copy link
Member

tvdeyen commented May 25, 2016

Yes, this is faster, but the UTC timestamp thing is better done in Rails and since .all.each is not domain logic, but framework defaults, this is ok IMO

Superseded by #1030

@tvdeyen
Copy link
Member

tvdeyen commented May 25, 2016

@pascalj @mamhoff please close, if you agree

@pascalj
Copy link
Contributor Author

pascalj commented May 25, 2016

I don't really agree (we could get the time as String from Ruby and still use SQL), but I don't have time to change it right now.

@tvdeyen
Copy link
Member

tvdeyen commented May 26, 2016

superseded by #1027

@tvdeyen tvdeyen closed this May 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants