Add translate for country & state

dimidev commented 1 year ago

add mobility to country
add mobility to state (there is room for improvement)
add Greek locale for the gem

mrbrdo commented 1 year ago

Hi, thanks for this and sorry for the late reply, was a little busy with other things.

Checked the code, seems good, only 2 things:

could you do StatesControllerDecorator in the same way like the 2 decorators in the last commit? I found there can be problems doing it like you did it, hence why I namespaced all decorators in CoreExt.
as I am trying to keep this gem working with Spree 4.3.x and 4.4/main, can you please check that it works with both versions?

Other than that looks good to me. 👍

dimidev commented 1 year ago

I've made some changes. please check it out. It also works with spree 4.4.0 .

i've changed the route to this to be able use TranslationsController inheritance in StateTranslationsController because states is nested resource inside countries. The :state_translations route contains :resource param, otherwise some controller method could not work properly. Such as resource_name and klass.

config/routes.rb

get '/countries/:country_id/:resource/:resource_id/translations' => 'state_translations#index', as: :state_translations

app/views/spree/admin/translations/state.html.erb Because state resource is nested in country i had to user @resource.country

...
<%= form_for [:admin, @resource.country, @resource], as: :state do |f| %>
      <%= render 'form_fields', f: f %>
      <%= render 'spree/admin/shared/edit_resource_links' %>
<% end %>
...

app/overrides/spree/admin/states/_state_list/add_translation.rb

...
<%= link_to_with_icon 'translate', nil, spree.admin_state_translations_path(state.country, 'state', state.id), title: Spree.t(:'i18n.translations'), class: 'btn btn-sm btn-primary', no_text: true %>
...

app/controllers/spree/admin/state_translations_controller.rb I had to overide collection_url method, because the original use resource_name that returns 'states' and we need country_states.

def collection_url
    send "admin_country_states_url", @resource.country
end

dimidev commented 1 year ago

Also i added if..else in country and state migration if timestamps is null in commit https://github.com/mrbrdo/spree_mobility/pull/11/commits/b71901a9e45bbb538865eb10569f52d8103ff8e4 because in spree 4.4.0 seed the timestamps at country and state tables is null.

mrbrdo commented 1 year ago

@dimidev very good job, thanks. And sorry it took a little bit for me to find time to review it.

mrbrdo commented 1 year ago

@dimidev there is one issue with your PR, you added this: ['created_at', 'updated_at'].include?(field_name), however, field_name is never that because you left this as is: migrate_data(Spree::State, params) - and mind that field_names = fields.keys (fields = params)

On my machine db:migrate failed due to this (created_at null).

Did you forget to add something?

mrbrdo / spree_mobility

Add translate for country & state #11