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

fix spelling of locale logged_in_successfully #3346

Merged
merged 2 commits into from
Nov 27, 2019

Conversation

nspinazz89
Copy link
Contributor

Description
Fix for #3345

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)

@jacobherrington
Copy link
Contributor

So this is a bit more complicated than I'd like it to be.

I think we are okay to merge this, but we will need to make a similar change in solidus_auth_devise.

It seems like this is something people have used for some time (based on a GitHub search). Hopefully the changelog will be enough to help people find this...

@jacobherrington
Copy link
Contributor

Here are all of the places where we have used the misspelling: https://github.com/search?q=org%3Asolidusio+logged_in_succesfully&type=Code

@kennyadsl
Copy link
Member

@kennyadsl
Copy link
Member

Just commented on solidusio/solidus_auth_devise#173, and now I have other concerns (sorry): if a user updates solidus without updating solidus_auth_devise they'll end up having no translation for the old wrong string which is still used into solidus_auth_devise. The worst thing is that in this scenario the i18n fallback will auto-generate a wrong "Logged in succesfully"string.

That said, I'm tempted to ask you to leave both strings, but the solution proposed here won't have any effect at that point.

Maybe the safest thing for users is to keep both things (double keys here (and solidus_i18n) and the fallback default in solidusio/solidus_auth_devise#173). This way, users that update solidus will have an extra string in the i18n and in the future (maybe solidus 3.0), we will be able to safely remove the duplicated (wrong) string. What do you think?

@spaghetticode
Copy link
Member

I agree with @kennyadsl, in order to play nice with every scenario we need to keep also the old key, and only later with a major release remove the misspelled one.

@kennyadsl
Copy link
Member

@nspinazz89 can you please rebase this one?

@nspinazz89 nspinazz89 force-pushed the bug/spelling-error-fix branch from 847824e to 7a39977 Compare October 24, 2019 16:23
Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

@nspinazz89 thank you 👍

@spaghetticode
Copy link
Member

@nspinazz89 In order to improve git history readability, can you squash the commits into a single one? 🙏

@kennyadsl kennyadsl merged commit 34b93c1 into solidusio:master Nov 27, 2019
nspinazz89 added a commit to geminimvp/solidus_auth_devise that referenced this pull request Feb 13, 2020
Add default value to ensure existing projects do not break.
solidusio#173

Add default value for logged_in_successfully for i18n

Add comment explaining purpose of change

This change is in support of
solidusio/solidus#3346
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.

4 participants