Add cookie support v2#399
Conversation
… use in request lifecycle
…anguagePreferenceCollection
|
|
||
| def process_locale(self, locale_code, source_of_evidence, **kwargs): | ||
| """ | ||
| How the locales are procssed for a user language preference. This method SHOULD |
There was a problem hiding this comment.
procssed => processed
| try: | ||
| discussion_id = req.matchdict['discussion_id'] if (req.matchdict and 'discussion_id' in req.matchdict) else None | ||
| discussion_id = req.matchdict['discussion_id'] if ( | ||
| 'matchdict' in req.__dict__ and req.matchdict and 'discussion_id' in req.matchdict) else None |
There was a problem hiding this comment.
This feels hacky and complicated. matchdict really can be missing in req?
I tend to prefer .get and gettattr syntax instead of in like you did. This line should do the same:
discussion_id = req.matchdict.get('discussion_id', None) if getattr(req, 'matchdict', None) is not None else None
If we always have a req.matchdict attribute, an empty dict or None, even simpler:
discussion_id = req.matchdict.get('discussion_id', None)
| return self.default_locale | ||
|
|
||
| def find_locale(self, locale, db=None): | ||
| def find_locale(self, locale, **kwargs): |
There was a problem hiding this comment.
Was it necessary to change db to kwargs here?
There was a problem hiding this comment.
Ooops, bad copy/pasta. I had previously changed the interface of having find_locale, but it simply caused way too many issues with translation services and post creation, so I reverted my changes. Missed this!
|
|
||
| # Query a property for fetching a user's preferences on a post | ||
| def __repr__(self): | ||
| string_rep = super(PostUserLanguagePreference, self).__repr__() |
There was a problem hiding this comment.
You shouldn't call __repr__() yourself, use the built-in repr(super(PostUserLanguagePreference, self))
There was a problem hiding this comment.
Fair, are there triggers that get called this way?
There was a problem hiding this comment.
yes, same for len which call len, str which call str etc.
| source_of_evidence=LanguagePreferenceOrder.Server.value) | ||
|
|
||
| def fin(): | ||
| print "finalizer user_language_preference_en_cookie" |
There was a problem hiding this comment.
This doesn't match the fixture name.
| first_original().locale_code == 'fr' | ||
|
|
||
|
|
||
| @pytest.mark.xfail(reason="IntegrityError on teardown on language_preference. TODO resolve!") |
There was a problem hiding this comment.
Did you try to resolve it?
There was a problem hiding this comment.
No, this one I will have to look at with @maparent . It likely has to do with the global Locale's, but for the time being, marked it as xfail
|
@ayyazdaniaryan What do we do with this PR? |
| ) | ||
|
|
||
|
|
||
| class PostUserLanguagePreference(UserLanguagePreference): |
There was a problem hiding this comment.
I think we need to set up a centralized system for user preferences. Currently, for the V1 (and the V2), we have models.preferences.Preferences (Json) and UserPreferenceCollection a wrapper for reading and writing data. This system is centralized, extensible and maintainable but is not sufficiently expressive (for complex needs). We also have an ad hoc system. In this case the preferences are decentralized and not easily maintainable but sufficiently expressive. I think we need to have a centralized system for user preference. For example, a table of preferences 'Preference' (not Json but can contain a Json) to describe complex needs. In this table we add the column 'language_preference' (for your need) which references an entry in a table 'Translation' and another column 'harvesting_translation' which references another entry in the same table 'Translation'. And we can add the Cookies preferences in this 'Preference' table.
4 participants
More tests to be added and front-end updates. However, a review is needed at this point.