nephila / djangocms-installer

Console wizard to bootstrap django CMS projects
https://djangocms-installer.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
177 stars 78 forks source link

I18n URL patterns regardless of I18n setting #321

Closed marksweb closed 6 years ago

marksweb commented 6 years ago

I've just setup a project with 1.0.0rc1 & opted for I18n to be disabled, but the URLs get generated with i18n patterns, resulting in a 404 after I've logged in as it loads /en/ for example.

Looks like the app needs two versions of urls.py for the two outcomes of the i18n setting. This looks like a simple change in copy_files() so I might fork the repo & try to resolve this.

yakky commented 6 years ago

@marksweb that would be awesome!

marksweb commented 6 years ago

@yakky no problem.

I've only looked at the code for the first time since raising this, but hopefully I've got the right idea as to how it all works; https://github.com/nephila/djangocms-installer/compare/develop...marksweb:feature/non-i18n-urls?expand=1

Got 2 failing tests at the moment, so just trying to see why before checking coverage & trying to test for i18n enabled & disabled as to what file gets copied.

yakky commented 6 years ago

@marksweb looks good. I'd just keep the files in the same directory naming something like urls_i18n.py, urls_noi18n.py as the target for shutil.copy includes the filename. This will keep it consistent. Which tests are failing? installer tests are kind of tricky, unfortunately

marksweb commented 6 years ago

@yakky ok, that makes sense, thanks.

These two are failing at the moment, but I can't see yet how anything I've done might have caused it.

======================================================================
FAIL: test_base_invocation (tests.main.TestMain)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mwalker/Documents/GitHub/djangocms-installer/tests/main.py", line 117, in test_base_invocation
    self.assertTrue(('Get into "%s" directory and type "python manage.py runserver" to start your project' % project_dir) in self.stdout.getvalue())
AssertionError: False is not true

======================================================================
FAIL: test_main_invocation (tests.main.TestMain)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mwalker/Documents/GitHub/djangocms-installer/tests/main.py", line 98, in test_main_invocation
    self.assertTrue(('Get into "%s" directory and type "python manage.py runserver" to start your project' % project_dir) in self.stdout.getvalue())
AssertionError: False is not true

----------------------------------------------------------------------
Ran 38 tests in 604.097s

FAILED (failures=2)
yakky commented 6 years ago

@marksweb checking briefly. you can eventually remove the asserts at the end of the test and add a couple of prints in the tests:

print(self.stdout.getvalue())
print(self.stdout.geterr())

I'll pull your branch to check myself

yakky commented 6 years ago

@marksweb it appears to be fixed in our branch, right?

marksweb commented 6 years ago

@yakky I've just ran the test suite from the develop branch & got the same 2 failures.

Not sure if there is something funny going on with my mac. Essentially I've cloned the project, created a venv for it, installed requirements-test.txt to that venv, then ran python setup.py test from the project root.

That should do what we want from a python project and run your test suite in an isolated env, using the python packages in that test requirements. I'm a little confused as to why it's failing.

In my branch, I've added two tests in main.py for i18n turned on, and turned off. Both pass for me, and I've setup a project using my version & with i18n disabled, I've got a standard urls.py file without the translated patterns. So at least that side of things is working as expected!

yakky commented 6 years ago

Locally they run ok Can you open a pull request anyways? Let's see what Travis says

The test procedure it's correct (you can also use tox -epyXY to automate the tests)