search

solidusio / solidus_auth_devise

🔑 Devise authentication for your Solidus store.
http://solidus.io
BSD 3-Clause "New" or "Revised" License
53 stars 128 forks source link

Fix spelling of locale logged_in_successfully #173

Closed nspinazz89 closed 3 years ago

nspinazz89 commented 4 years ago

This addresses solidusio/solidus#3346 and ensures existing projects will not break by providing a default.

kennyadsl commented 4 years ago

Thanks, @nspinazz89! I'd love to fix this issue but I have a concern: if we change this key and users update solidus_auth_devise without also updating solidus to a version that contains #3346, they will end up not having the logged_in_successfully translation in their store, right? The "good" thing is that if that key is missing the fallback should be the right "Logged in successfully", but we can't handle if users changed the key in their application.

The same concept can be applied for who is not using en locale only and uses https://github.com/solidusio/solidus_i18n.

What about still checking if the old string exists before using the new one with:

I18n.t('spree.logged_in_succesfully', default: :'spree.logged_in_successfully')
nspinazz89 commented 4 years ago

Thanks for the feedback @kennyadsl, alternatively we could just add it as an extra option in the yaml file. I'm good with either though!

kennyadsl commented 4 years ago

@jacobherrington I agree this could be documented better. The commit message should explain why we are doing this change and we should probably also link https://github.com/solidusio/solidus/pull/3346 in the PR description.

kennyadsl commented 4 years ago

I'm a bit hesitant merging this since I'd prefer to see that comment text in the commit description instead.

spaghetticode commented 4 years ago

@nspinazz89 can you improve a bit the commit message, so it's more clear why we're doing this? After that, I think this is good to merge!

nspinazz89 commented 4 years ago

@kennyadsl and @spaghetticode is this sufficient or are there some other changes I should make as well?

kennyadsl commented 4 years ago

I still think we should remove code comments if we moved in the commit description. Thanks!