-
Notifications
You must be signed in to change notification settings - Fork 0
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
add time_zone configutation to users #11
base: release/0.26-stable
Are you sure you want to change the base?
add time_zone configutation to users #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @antopalidi
this is an initial review, but you can already apply (some) of the changes. I'll fully test it later and give you more feedbacks and hints on how to proceed with tests
decidim-core/app/controllers/concerns/decidim/use_organization_time_zone.rb
Outdated
Show resolved
Hide resolved
decidim-core/db/migrate/20220823094517_add_time_zone_to_users.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @antopalidi
It seems that default values on the account_form.rb
cannot extract data from the context, instead we can add the default value in the view (as suggested in the comments).
Also, it is not necessary to enforce the presence of a timezone to the user, we will just use the organization if not present.
We also need to add a test case in the file decidim-core/spec/controllers/concerns/use_organization_time_zone_spec.rb
and add a case for when the time zone is defined by the user and see that it takes precedence over the organizaion (only if present)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
If left a comment, please check it out.
On the other hand, we need to look at the failing tests, the first one is about linting the YAML locale file, this can be done automatically by executing:
bundle exec i18n-tasks normalize --locales en
The second one is because (I think) you need to add
let(:timezone) { "UTC" }
at the same level of the let(:data)
in the file update_account_spec.rb
decidim-core/app/controllers/concerns/decidim/use_organization_time_zone.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic @antopalidi !
I've added an additional test, but that's it!
ready to deploy!
Adds configurable timezone for users in order to make Decidim more usable for multi-zone organizations/countries
fixes #10