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 store empty values #32

Closed

Conversation

saippuakauppias
Copy link
Contributor

Я абсолютно не уверен в том, правильно ли я всё сделал, но по-другому мне не удалось его заставить работать: или при самом первом открытии страницы (куки нету) не выбирался нужный location, или при попытке обнуления оставался location, который берётся из ip (не куки).

Возможно, нужно было бы что-то сделать с условием из storage.py:

        # Cookie is obsolete, because we've changed it's value during request
        if str(self._get_location_id()) != str(new_value):
            return True

Но из-за того, что в Locator было условие:

        if not stored_location:

пустое значение (GEOIP_LOCATION_EMPTY_VALUE, по дефолту равное 0), заставляло брать значение не из cookies, поэтому при смене региона на "все регионы" ничего не получалось :)
Да и process_response из-за первого и второго условия устанавливал регион из ip, когда регион у меня был сброшен в "пустой". К слову, не очень понятно зачем он там был нужен :)

И ещё, к слову, у меня в кастомной LocationModel методы класса выглядят вот так (если принимать pull-request, то и в документации что-то об этом надо дописать):

class CustomRegion(GeoLocationFacade, ...):
    ...
    is_enabled = models.BooleanField(_('enabled'), default=True)
    geoip_region = models.OneToOneField(Region, related_name='custom_region',
                                        blank=True, null=True,
                                        verbose_name=_('GeoIP Region'))

    @classmethod
    def get_by_ip_range(cls, ip_range):
       # try/except тут потому что есть ситуации, когда регион не привязан к модели
        try:
            region = ip_range.region.custom_region
        except AttributeError:
            raise CustomRegion.DoesNotExist
        else:
            return region

    @classmethod
    def get_default_location(cls):
        return settings.GEOIP_LOCATION_EMPTY_VALUE

    @classmethod
    def get_available_locations(cls):
        # а вот без третьего коммита можно было установить регион через куку, который нельзя установить
        return cls.objects.filter(is_enabled=True)

PS: оставил с поломавшимися тестами, т.к. совершенно не понятно прав ли я в своём пулл-реквесте, он очень спорный, но рабочий для меня.
PPS: пока буду юзать свою ветку, возможно скоро даже в продакшене.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 528770f on saippuakauppias:fix_store_empty_values into 4b72427 on futurecolors:dev.

@coagulant
Copy link
Member

Можешь написать тест, который бы проваливался относительно 0.3.1 и воспроизводил бы описанную тобой ошибку?

@saippuakauppias
Copy link
Contributor Author

Хорошо, постараюсь чуть позже это сделать (на выходных или после)

@coagulant
Copy link
Member

Спасибо.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling 99167d4 on saippuakauppias:fix_store_empty_values into 4b72427 on futurecolors:dev.

@saippuakauppias
Copy link
Contributor Author

Сейчас ещё было замечена такая неприятная особенность, что т.к. в Locator._get_corresponding_location на строке:

    return location_model.get_by_ip_range(ip_range)

никак не взаимодействует с location_model.get_available_locations, что в результате приводит к тому, что если я зашел (без кук, т.е. первый раз) с не доступного, но существующего региона, то отобразится именно он. Очень криво (как мне кажется) это удалось обойти, изменив метод get_by_ip_range на:

    def get_by_ip_range(cls, ip_range):
       # try/except тут потому что есть ситуации, когда регион не привязан к модели
        try:
            region = ip_range.region.custom_region
        except AttributeError:
            raise CustomRegion.DoesNotExist
        else:
+            if not region.is_enabled:
+                raise CustomRegion.DoesNotExist
            return region

Этого можно было бы избежать, если бы в методе Locator._get_corresponding_location в указанной выше строке была бы ещё фильтрация через location_model.get_available_locations. Я не стал этого делать, т.к.:

  1. это метод класса;
  2. возможно подразумевается, что оно должно обрабатываться именно так, силами кастомного класса LOCATION_MODEL;
  3. это приличное backwards incompatible change.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling 24b88b6 on saippuakauppias:fix_store_empty_values into 4b72427 on futurecolors:dev.

@coagulant
Copy link
Member

Всё верно, "оно должно обрабатываться именно так, силами кастомного класса LOCATION_MODEL". get_available_locations это просто для выпадайки регионов метод. Я допишу в доки про это.

@coagulant
Copy link
Member

Тесты сломаны. Если будет желание вернуться к починке - я помогу чем смогу.

@coagulant coagulant closed this Aug 23, 2014
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.

None yet

3 participants