Migrate Edraak i18n platform #38

OmarIthawi · 2017-02-22T07:50:09Z

Task:

Migrate edraak_i18n module with a feature flag

Things wasn't migrated:

Transifex theme command, will be migrated with the Transifex task.

Salomari1987 · 2017-03-05T16:32:33Z

cms/envs/aws.py

+    import edraak_i18n.helpers
+    INSTALLED_APPS += ('edraak_i18n',)
+    MIDDLEWARE_CLASSES = edraak_i18n.helpers.add_locale_middleware(MIDDLEWARE_CLASSES)
+


I like this

Glad you do 👍

Salomari1987 · 2017-03-06T08:57:29Z

common/djangoapps/edraak_i18n/middleware.py

+    Should be installed *before* any middleware that checks request.META['HTTP_ACCEPT_LANGUAGE'],
+    specifically django.middleware.locale.LocaleMiddleware
+
+    Although the middleware is installed by default, it sill checks for the feature flag bellow in order to work.


Spelling mistakes:

bellow is below (line 19)

the the (line 11)

Salomari1987 · 2017-03-06T08:59:59Z

common/djangoapps/edraak_i18n/helpers.py

+    elif request.path.startswith('/user_api/'):
+        return True
+    elif request.path.startswith('/notifier_api/'):
+        return True


You can use regex here to reduce lines of code

Yes, but I'd like to be very specific and it is more readable for me this way.

its not generic though, if more urls are introduced in the future that represent api requests, then the decision tree becomes hideous, no?

It's not generic, and we'll wait for it to break in the future, we still rely on manual QA for e2e tests.

While the regexp is a valid suggestion, it's not guaranteed to work.

For example edX could add any of these:

/certificate_api/

/xmodule/api/

There's still some guessing and the code won't be future proof anyway, in this case I always fallback to the stupid solution i.e. simple if statement vs. a RegExp.

Salomari1987 · 2017-03-06T09:16:47Z

cms/envs/test.py

+
+# Enable Edraak apps for testing
+# Keep it in sync with {lms,cms}/envs/{test,aws}.py
+import edraak_i18n.helpers


use from ... import

This is actually better, to avoid polluting the settings.

Salomari1987 · 2017-03-06T09:52:19Z

common/djangoapps/edraak_i18n/tests.py

+            updated_middleware.index('edraak_i18n.middleware.DefaultLocaleMiddleware'),
+            updated_middleware.index('django.middleware.locale.LocaleMiddleware'),
+            'The i18n middleware should come before the `LocaleMiddleware`',
+        )


This is already tested before, no need to repeat again

Salomari1987 · 2017-03-06T09:53:50Z

common/djangoapps/edraak_i18n/helpers.py

+    first_index = min(indexes)
+
+    # Insert the DefaultLocaleMiddleware any other locale-related middleware in order for it to work
+    return middleware_classes[:first_index] + (edraak_middleware,) + middleware_classes[first_index:]


You can do another logic with less complexity, check for the first element, if it is any of the (other_local_middlewares), then add the middleware as the first element, otherwise, make it the second, or you could always add it as the first, what do you think?

What about the SessionMiddleware or the SafeSessionMiddleware? locale middleware requires one of them to work.

Besides it's much more defensive to write it this way!

But here you are not checking for SessionMiddleware or any other middleware that Locale middleware depends on, thus defensive programming does not exactly apply here, maybe if there was a function that checks middlewares and rearranges them correctly, what do you think?

OmarIthawi · 2017-03-06T12:50:17Z

All comments have been addressed. Please let me know if you have more.

Salomari1987 · 2017-03-06T13:22:59Z

@OmarIthawi sounds good, maybe you can add a TODO: write more comprehensive tests

OmarIthawi · 2017-03-06T14:52:08Z

@OmarIthawi sounds good, maybe you can add a TODO: write more comprehensive tests

The edX default setup hates TODOs and the build will fail, so we won't write any TODOs.

If you have a suggestion for a specific missing test, please suggest. Otherwise, we'll go with what we have.

OmarIthawi · 2017-03-08T07:06:30Z

@Salomari1987 Let's wait for #39 to get merged so we can get rid off the failing flaky tests.

Salomari1987

Please update this branch with master

OmarIthawi · 2017-03-16T10:47:07Z

@Salomari1987 the tests should pass now.

TL;DR

I've actually cherry-picked the flaky test fixups here as well. If either one is merged, we'll remove the cherry-picks from the others and rebase.

Salomari1987 · 2017-03-16T13:29:17Z

@OmarIthawi PR Ratelimit is merged, so you can rebase here and reference me to merge

OmarIthawi · 2017-03-18T15:24:46Z

Thanks @Salomari1987. Just did.

…h quality fixes

OmarIthawi · 2017-03-19T16:23:31Z

@Salomari1987 it passed CI tests.

!فرصة تراجعها قبل ما يغير رأيه

OmarIthawi · 2017-03-27T09:38:20Z

😄 الله يسعدك رفيق

OmarIthawi changed the title ~~Migrate Edraak i18n platform~~ (WIP) Migrate Edraak i18n platform Feb 22, 2017

OmarIthawi force-pushed the omar/migrate-edraak-i18n-module branch from fedb7a5 to d347204 Compare February 26, 2017 08:38

OmarIthawi changed the title ~~(WIP) Migrate Edraak i18n platform~~ Migrate Edraak i18n platform Feb 26, 2017

OmarIthawi requested a review from Salomari1987 February 26, 2017 08:44

Salomari1987 suggested changes Mar 6, 2017

View reviewed changes

OmarIthawi force-pushed the omar/migrate-edraak-i18n-module branch from d347204 to d86427f Compare March 6, 2017 12:49

OmarIthawi force-pushed the omar/migrate-edraak-i18n-module branch from d86427f to 02ec26c Compare March 6, 2017 15:17

Salomari1987 approved these changes Mar 16, 2017

View reviewed changes

Salomari1987 suggested changes Mar 16, 2017

View reviewed changes

OmarIthawi force-pushed the omar/migrate-edraak-i18n-module branch from 02ec26c to 0526d78 Compare March 16, 2017 10:45

OmarIthawi force-pushed the omar/migrate-edraak-i18n-module branch from 0526d78 to 255db86 Compare March 18, 2017 15:14

OmarIthawi force-pushed the omar/migrate-edraak-i18n-module branch 3 times, most recently from b5a6eb8 to 6c66961 Compare March 19, 2017 11:46

Migrate the edraak_i18n module from the dogwood repo with tests wit…

62cc58f

…h quality fixes

OmarIthawi force-pushed the omar/migrate-edraak-i18n-module branch from 6c66961 to 62cc58f Compare March 19, 2017 12:40

Salomari1987 approved these changes Mar 27, 2017

View reviewed changes

Salomari1987 merged commit a3c97d7 into master Mar 27, 2017

OmarIthawi deleted the omar/migrate-edraak-i18n-module branch August 24, 2017 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Edraak i18n platform #38

Migrate Edraak i18n platform #38

OmarIthawi commented Feb 22, 2017 •

edited

Loading

Salomari1987 Mar 5, 2017

OmarIthawi Mar 6, 2017

Salomari1987 Mar 6, 2017

Salomari1987 Mar 6, 2017

OmarIthawi Mar 6, 2017

Salomari1987 Mar 6, 2017

OmarIthawi Mar 6, 2017 •

edited

Loading

Salomari1987 Mar 6, 2017

OmarIthawi Mar 6, 2017

Salomari1987 Mar 6, 2017

OmarIthawi Mar 6, 2017

Salomari1987 Mar 6, 2017

OmarIthawi Mar 6, 2017

Salomari1987 Mar 6, 2017

OmarIthawi commented Mar 6, 2017

Salomari1987 commented Mar 6, 2017

OmarIthawi commented Mar 6, 2017 •

edited

Loading

OmarIthawi commented Mar 8, 2017

Salomari1987 left a comment

OmarIthawi commented Mar 16, 2017

Salomari1987 commented Mar 16, 2017

OmarIthawi commented Mar 18, 2017

OmarIthawi commented Mar 19, 2017

OmarIthawi commented Mar 27, 2017

Migrate Edraak i18n platform #38

Migrate Edraak i18n platform #38

Conversation

OmarIthawi commented Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarIthawi Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarIthawi commented Mar 6, 2017

Salomari1987 commented Mar 6, 2017

OmarIthawi commented Mar 6, 2017 • edited Loading

OmarIthawi commented Mar 8, 2017

Salomari1987 left a comment

Choose a reason for hiding this comment

OmarIthawi commented Mar 16, 2017

TL;DR

Salomari1987 commented Mar 16, 2017

OmarIthawi commented Mar 18, 2017

OmarIthawi commented Mar 19, 2017

OmarIthawi commented Mar 27, 2017

OmarIthawi commented Feb 22, 2017 •

edited

Loading

OmarIthawi Mar 6, 2017 •

edited

Loading

OmarIthawi commented Mar 6, 2017 •

edited

Loading