seixasfelipe / cleaners

Cleaners - Management Application
MIT License
2 stars 0 forks source link

Tests should be country isolated #21

Closed fraga closed 11 years ago

fraga commented 11 years ago

We cannot rely on specific country on our tests, we should isolate it, specially when we talk about labels.

It's still confusing for me the division on this app when we talk about the devise login gem. It has a lot of "labels", or translations, we could reuse (DRY) but also I don't know how we could leverage that.

fabianoalmeida commented 11 years ago

Nice catch @fraga! I usually did it in my projects and I was studying do it soon but you were more fast than me. :blush:

fabianoalmeida commented 11 years ago

Fixed in #22.

fraga commented 11 years ago

@fabianoalmeida I cannot be faster than you =) another thing is we need (maybe I need) to understand how can we reuse devise yml localization. And yet, we still have a lot of tests that still looks for hardcoded country specific links...

fraga commented 11 years ago

We have more tests to fix so I'm reopening this issue

fabianoalmeida commented 11 years ago

@fraga Oops! Yeah, I'm so sorry. :blush:

fraga commented 11 years ago

it appears that devise gem does not fully support internationalization, am I correct? What I mean is that you have some hardcoded translations in there...

fabianoalmeida commented 11 years ago

@fraga If we define translation like t("some.text") or I18n.t("some.text") this method will try to match it in the .yml file correspondent. In this case, devise.#{ en | pt-BR }.yml file.

We need put this match there to avoid some wrong behavior as mentioned by @seixasfelipe in #22. So, I can't see that it's necessary some hardcoded translations, it's really simple in my point of view. Or I missed something? :confused:

fraga commented 11 years ago

@fabianoalmeida that's what I mean. Look at some views under devise, they've been hardcoded...

fraga commented 11 years ago

@fabianoalmeida check this file out for instance app/views/devise/registrations/edit.html.erb Did we create it or it's part of devise gem?

fraga commented 11 years ago

@fabianoalmeida you can note that that file has hard coded "SIgn in". So I don't know if devise has different views per country (didn't note that) but at least it's not using the I18n.t

fabianoalmeida commented 11 years ago

@fraga all views under app/views/devise were created after that I ran the command rails generate devise:views. Now I understood what you meant with hardcoded. So, we need modify all views generated by devise gem to solve problems like you showed and these views are the default.

fraga commented 11 years ago

@fabianoalmeida so, devise does not leverage internationalization on the rails script creation (wonder if they could change that). I don't like the way rails divide labels files, it breaks DRY principle. Why would we have devise.pt-br, devise.en and also our specific labels en and pt-br since we could leverage the labels already created.

fabianoalmeida commented 11 years ago

@fraga yes, you're correct and I wonder it too. The installation devise process identify some gems already, like simple_form, responders, etc. But doesn't do that with I18n. I don't know why though this could be awesome! When I have more free time I'll try create a patch to suggest it for them. :wink:

seixasfelipe commented 11 years ago

I agree with you guys, but I also understand why they would divide into many translations files (.yml). Sometimes you have specific messages to some part of your app or some component (which is the case of devise, responders, etc).. So, when a Rails app starts, it loads all those files and store in memory as one list of translations key/value pairs.

In conclusion, many files help to organize specific messages, that what I think about it.

On Fri, Mar 8, 2013 at 12:59 PM, Fabiano Almeida notifications@github.comwrote:

@fraga https://github.com/fraga yes, you're correct and I wonder it too. The installation devise process identify some gems yet, like simple_form, responders, etc. But doesn't do that with I18n. I don't know why though this could be awesome! When I have more free time I'll try create a patch to suggest it for them. [image: :wink:]

— Reply to this email directly or view it on GitHubhttps://github.com/seixasfelipe/cleaners-panamby/issues/21#issuecomment-14633863 .

fraga commented 11 years ago

So I have the following problem for you guys - devise created views without internationalization, what should I do?

1) Fix their error on the creation of the views 2) Create missing labels under devise yml files 3) Create missing labels under our yml files

All of our tests passes because we hard coded portuguese into it. Try running the tests under "en" you will see what happens =)

fabianoalmeida commented 11 years ago

@fraga IMO the option 3 it's more wise for our internationalization terms. Cause we need translate devise texts, we should be created under devise yml files.

fraga commented 11 years ago

Never tried yet but does I18n matches labels that are not inside 'devise' yml files? So if you create a label in en.yml (our label) and I go to devise view and point to t('helpers.submit.edit') will it understand that it needs to take from our labels?

seixasfelipe commented 11 years ago

It works, I've tested here. Rails loads all files that were configured inside application.rb as translations files and merges them into one file in memory (I guess).

fraga commented 11 years ago

You tested under which language, en or pt-br?

seixasfelipe commented 11 years ago

Both :smile:

fraga commented 11 years ago

How are you guys testing?

seixasfelipe commented 11 years ago

I run rspec and everything is fine. Of course I had to also change sign_up_spec.rb

click_link 'Criar usuario'

to:

click_link I18n.t('helpers.links.create')

Another thing that I did was passing a locale parameter when visiting root_path

visit root_path(locale: 'en')

And Voilà!

fraga commented 11 years ago

Can you specify country on rspec?

fraga commented 11 years ago

I mean on the command line

seixasfelipe commented 11 years ago

I don't know if we could pass parameters to rspec.

fabianoalmeida commented 11 years ago

I put autotest-rails gem in gemfile, so we can run autotest on command line to test all specs. Cause any error appear, we can solve it and the gem will test the specs again, automatically. I'm sorry, I could have said it before. :blush:

fraga commented 11 years ago

So please guys, from now on, tests should not have hard coded labels =)

fabianoalmeida commented 11 years ago

As suggested by @fraga in #30, closing.