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

improvements to i18n, pgsql compat and validations #28

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

sbeam
Copy link

@sbeam sbeam commented Sep 13, 2012

this is some various stuff in one PR, but it's a worthy one IMO.

  • i18n keys were missing in admin, German trans added.
  • columns names 'from' and 'to' make postgres unhappy
  • events need to have dates, and they need to end after they begin

tests should pass

jeremiahishere and others added 16 commits June 29, 2012 15:10
The from and to fields were changed to start_at and end_at for postgresql
compatibility.  The forms have been updated for the new fields.  The
locales still call the fields "From" and "To" but are now referenced with
:start_at and :end_at.

The two fields were also change from date to datetime fields.  I have
not included an upgrade migration for either database change.
* Fixed data types on migration.
* Removed uniqueness constraint on Event.
* Fixed a bad ordering on Event.
fixed conflict with events#show needing new column names

Conflicts:
	app/views/refinery/calendar/events/show.html.erb
there is no reason events can't have the same titles
validates start and end date are present, and in the logical order.

It would seem to be the definition of an event - something that occurrs
over a given time. So, it's resonable to require times.
@@ -12,8 +12,8 @@ Gem::Specification.new do |s|
s.files = Dir["{app,config,db,lib,vendor}/**/*"] + ["readme.md"]

# Runtime dependencies
s.add_dependency 'refinerycms-core', '~> 2.0.3'
s.add_dependency 'refinerycms-core', '~> 2.0'
Copy link
Member

Choose a reason for hiding this comment

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

so you're saying this now works with all versions of Refinery up until 3.0?

Copy link
Author

Choose a reason for hiding this comment

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

well of course - I can't know that - but I'd hope so :) With the ~> operator it seems to me no need to lock it down so much since we don't know if 2.1 will break this, so no need to force people in the future to deal with this dependency. Just depends on how conservative you prefer to be I guess, totally up to you.

Copy link
Member

Choose a reason for hiding this comment

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

I am wildly conservative ;-) I've been bitten way too many times in the past.

@parndt
Copy link
Member

parndt commented Nov 22, 2012

@sbeam OK So this PR effectively replaces #25 and maybe #27 is that right? /cc @Tranquility @jeremiahishere @luis-mendo @brenes?

Sorry for the massive delay :(

@Tranquility
Copy link

It only replaces #25. The translation commits only deal with static strings but do not allow the user to add multiple descriptions for different languages.

@parndt
Copy link
Member

parndt commented Nov 23, 2012

@Tranquility thanks!

@milgner
Copy link

milgner commented Mar 21, 2013

+1 since the current master is broken on PostgreSQL (and supposedly SQLite, too).
Is a reversal of a267222 the only thing blocking this PR? Glancing over the other changes, everything looks good so far.

@brenes
Copy link

brenes commented Mar 26, 2013

with #38 merged master should be compatible again with Postgre and SQLite

@parndt parndt mentioned this pull request Jul 8, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants