Issue 10537043: Determining subscribers by subscription_ids (Closed)
DescriptionFor more information on subscription_ids format check EP (http://etherpad.osuosl.org/moin-mail) Section 1.3
The `subscribed_items` meta was not removed ATM, since it involves a lot of other changes unrelated to current cr.
Patch Set 1 #
Total comments: 13
Patch Set 2 : #
Total comments: 24
Patch Set 3 : #
Total comments: 3
Patch Set 4 : Fix the tests and add TypeError #
Total comments: 8
Patch Set 5 : Remove redundant check #Patch Set 6 : Remove TAG constant #Patch Set 7 : Change create_user usage in tests #
Total comments: 4
Patch Set 8 : Define Subscriber once #Patch Set 9 : Remove isinstance checks #
Total comments: 8
Patch Set 10 : #Patch Set 11 : Remove redundant imports #MessagesTotal messages: 21
https://codereview.appspot.com/10537043/diff/1/MoinMoin/storage/middleware/in... File MoinMoin/storage/middleware/indexing.py (right): https://codereview.appspot.com/10537043/diff/1/MoinMoin/storage/middleware/in... MoinMoin/storage/middleware/indexing.py:325: Why do you need the ids to be stored, at least for this change? https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py... MoinMoin/util/subscriptions.py:32: itemid_term = [Term(SUBSCRIPTION_IDS, ITEMID + ':' + item.meta[ITEMID])] Why a list? https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py... MoinMoin/util/subscriptions.py:34: for name in item.meta[NAME]] Why do you create a list here? https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py... MoinMoin/util/subscriptions.py:37: query = Or(itemid_term + name_terms + tag_terms) Doesn't "Or" get multiple arguments? https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py... MoinMoin/util/subscriptions.py:44: def get_users(results): You may want to read all informations you need (name, e-mail) from the index.
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/1/MoinMoin/storage/middleware/in... File MoinMoin/storage/middleware/indexing.py (right): https://codereview.appspot.com/10537043/diff/1/MoinMoin/storage/middleware/in... MoinMoin/storage/middleware/indexing.py:325: If ids are not stored then I can't get them from the index or search for them in the index. https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py... MoinMoin/util/subscriptions.py:32: itemid_term = [Term(SUBSCRIPTION_IDS, ITEMID + ':' + item.meta[ITEMID])] Just to concatenate successfully with other lists. Might be as well - `query = Or([itemid_term] + name_terms + tag_terms)` https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py... MoinMoin/util/subscriptions.py:34: for name in item.meta[NAME]] `name` meta is a list. An item can have multiple names, so we want to check all of them. https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py... MoinMoin/util/subscriptions.py:37: query = Or(itemid_term + name_terms + tag_terms) It gets subqueries - a list of Query objects. http://whoosh.readthedocs.org/en/latest/api/query.html?highlight=#whoosh.quer... https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py... MoinMoin/util/subscriptions.py:44: def get_users(results): We would like to have the User objects and not extract directly the emails or names, because we might need some other information about users (language i.e) to manipulate easier the email sending process.
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/1/MoinMoin/storage/middleware/in... File MoinMoin/storage/middleware/indexing.py (right): https://codereview.appspot.com/10537043/diff/1/MoinMoin/storage/middleware/in... MoinMoin/storage/middleware/indexing.py:325: On 2013/06/25 10:19:03, ana.balica wrote: > If ids are not stored then I can't get them from the index or search for them in > the index. What is the use of an index if you can't search for a value? Please read the whoosh documentation again about the meaning of the "stored" attribute. You never need to retrieve this value for now, only search for them. https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py... MoinMoin/util/subscriptions.py:34: for name in item.meta[NAME]] On 2013/06/25 10:19:03, ana.balica wrote: > `name` meta is a list. An item can have multiple names, so we want to check all > of them. Then you want to do something like ".extend((bla in bla))" without generating a lot of lists. https://codereview.appspot.com/10537043/diff/1/MoinMoin/util/subscriptions.py... MoinMoin/util/subscriptions.py:44: def get_users(results): On 2013/06/25 10:19:03, ana.balica wrote: > We would like to have the User objects and not extract directly the emails or > names, because we might need some other information about users (language i.e) > to manipulate easier the email sending process. Add all the information to the subscription index you need. You don't want to read all users just for fun.
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/19001/MoinMoin/constants/keys.py File MoinMoin/constants/keys.py (right): https://codereview.appspot.com/10537043/diff/19001/MoinMoin/constants/keys.py... MoinMoin/constants/keys.py:33: TAG = u"tag" ehrm, can we change tags to tag in a separate cs? maybe also some other plural keys that are a bit impractical (e.g. in search queries you want to say tag:sometag not tags:sometag. NAME is already singular (even though it is a list of names). https://codereview.appspot.com/10537043/diff/19001/MoinMoin/constants/keys.py... MoinMoin/constants/keys.py:70: SUBSCRIBED_ITEMS = u"subscribed_items" that should be removed, right? https://codereview.appspot.com/10537043/diff/19001/MoinMoin/storage/middlewar... File MoinMoin/storage/middleware/indexing.py (right): https://codereview.appspot.com/10537043/diff/19001/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/indexing.py:326: SUBSCRIPTION_IDS: ID(), hmm, this is also plural. guess it needs a global review/concept, whether we want plurals or not. https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_subscriptions.py (right): https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_subscriptions.py:35: assert set() == users can we have assert <got> == <expected value>? https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_subscriptions.py:40: subscriptions = [ITEMID + ':' + self.item.meta[ITEMID]] use a format string? (in general, also see below) https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_subscriptions.py:44: assert u1.profile._meta[ITEMID] in subscribers_itemids if you add the user name to the get_subscribers result, you could do the check simpler, just based on the name. there should be also some user who is NOT subscribed to anything to test against false positives. https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_subscriptions.py:92: return user_ i'ld rather like to see a more powerful user.create_user() than a lot of such specializations. try to do it in a generally useful way. https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:25: self.locale = locale maybe you want also the NAME list of the user, in case we want to show a name after the notifies were sent (we will never show the email address there). if you don't need more functionality than just storing stuff, use a namedtuple. https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:36: if not item and not isinstance(item, Item): why is that check needed? https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:44: for tag in item.meta[TAGS]) you are saying item.meta here too often. also, use format strings? https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:47: results = searcher.search(query) limit=? https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:61: if not results or not isinstance(results, Results): why is that check needed? https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:66: users.add(user) use a set comprehension {x for x in 'abracadabra' if x not in 'abc'}
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/19001/MoinMoin/constants/keys.py File MoinMoin/constants/keys.py (right): https://codereview.appspot.com/10537043/diff/19001/MoinMoin/constants/keys.py... MoinMoin/constants/keys.py:70: SUBSCRIBED_ITEMS = u"subscribed_items" right. but not at the moment. next step. there is a lot of code that is relying on this constant, hence this cr would contain a lot of bits from ~10 other files. https://codereview.appspot.com/10537043/diff/19001/MoinMoin/storage/middlewar... File MoinMoin/storage/middleware/indexing.py (right): https://codereview.appspot.com/10537043/diff/19001/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/indexing.py:326: SUBSCRIPTION_IDS: ID(), At least plurals reflect the reality and there are some of them (quicklinks, itemlinks, bookmarks). If we decide to change smth related to plurals, that would be a separate commit. https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_subscriptions.py (right): https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_subscriptions.py:35: assert set() == users ok. i will know for future that this is the default format. https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_subscriptions.py:44: assert u1.profile._meta[ITEMID] in subscribers_itemids So do all the checks based on names? Names and itemids checks are equivalent. https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:25: self.locale = locale store the multiple name (the list of names) or just the first one? https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:36: if not item and not isinstance(item, Item): if the incorrect parameter is passed, then it will break the below code, like accessing meta. same for the check in next function. or am i missing something? https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:47: results = searcher.search(query) limit=None default is 10 - missed that. https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:66: users.add(user) About storing name to the namedtuple: name is not hashable, so we can't store it in a set. Use just the first name? `result[NAME][0] ?
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_subscriptions.py (right): https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_subscriptions.py:44: assert u1.profile._meta[ITEMID] in subscribers_itemids On 2013/06/26 08:51:19, ana.balica wrote: > So do all the checks based on names? > Names and itemids checks are equivalent. well, it would test the whole thing in a rather easy way, not just half of it. of course you can also have separate additional tests the tests for itemids -> (name, email, locale) code. https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:25: self.locale = locale maybe the first name is enough https://codereview.appspot.com/10537043/diff/19001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:36: if not item and not isinstance(item, Item): well, that's a bit unusual. if you get something wrong (a param type that is not acceptable), you shouldn't silently fail just returning nothing, but it should crash. https://codereview.appspot.com/10537043/diff/29001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_subscriptions.py (right): https://codereview.appspot.com/10537043/diff/29001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_subscriptions.py:114: return u can't you just give kwargs to create_user? or a meta dict? https://codereview.appspot.com/10537043/diff/29001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/29001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:24: :rtype set :rtype: ?
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/29001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/29001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:24: :rtype set return type. or remove it?
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/38001/MoinMoin/constants/keys.py File MoinMoin/constants/keys.py (right): https://codereview.appspot.com/10537043/diff/38001/MoinMoin/constants/keys.py... MoinMoin/constants/keys.py:33: TAG = u"tag" hmm, another idea for that: instead of saying what it is (a tag), we could also say what field we search in (tags). so you don't need this duplication. later we can think about whether we want to change/normalize these plurals. https://codereview.appspot.com/10537043/diff/38001/MoinMoin/storage/middlewar... File MoinMoin/storage/middleware/indexing.py (right): https://codereview.appspot.com/10537043/diff/38001/MoinMoin/storage/middlewar... MoinMoin/storage/middleware/indexing.py:326: SUBSCRIPTION_IDS: ID(), yup, looks like we don't need to store that, just index. https://codereview.appspot.com/10537043/diff/38001/MoinMoin/util/_tests/test_... File MoinMoin/util/_tests/test_subscriptions.py (right): https://codereview.appspot.com/10537043/diff/38001/MoinMoin/util/_tests/test_... MoinMoin/util/_tests/test_subscriptions.py:107: return u i already commented on the above method and how to solve it better... https://codereview.appspot.com/10537043/diff/38001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/38001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:57: return users why do you need this is_empty check? what would happen if you leave it away?
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/38001/MoinMoin/constants/keys.py File MoinMoin/constants/keys.py (right): https://codereview.appspot.com/10537043/diff/38001/MoinMoin/constants/keys.py... MoinMoin/constants/keys.py:33: TAG = u"tag" It's a bit unclear to me. Do you suggest removing the singular tag and use only the plural form? https://codereview.appspot.com/10537043/diff/38001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/38001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:57: return users With or without, anyway an empty set will be returned. The check allows skipping building the namedtuple and trying to iterate. If you consider it redundant, I will remove it.
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/38001/MoinMoin/constants/keys.py File MoinMoin/constants/keys.py (right): https://codereview.appspot.com/10537043/diff/38001/MoinMoin/constants/keys.py... MoinMoin/constants/keys.py:33: TAG = u"tag" yes https://codereview.appspot.com/10537043/diff/38001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/38001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:57: return users On 2013/06/27 20:20:05, ana.balica wrote: > With or without, anyway an empty set will be returned. > The check allows skipping building the namedtuple and trying to iterate. If you > consider it redundant, I will remove it. well, it makes the code more complex and the result would be same anyway. so remove it. without it, the code in this function shrinks to 1 effective line, so you can even think about inlining it...
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/52001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/52001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:27: raise TypeError("item should be an instance of MoinMoin.items.Item") Why do you need this check? https://codereview.appspot.com/10537043/diff/52001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:51: raise TypeError("results should be instance of whoosh.searching.Results") Why do you need this check? https://codereview.appspot.com/10537043/diff/52001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:53: Subscriber = namedtuple('Subscriber', [ITEMID, NAME, EMAIL, LOCALE]) You don't want to re-define there every time.
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/52001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/52001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:27: raise TypeError("item should be an instance of MoinMoin.items.Item") To tell explicitly the user what kind of object the function is expecting, in case something else is passed than Item. Same idea below.
Sign in to reply to this message.
On 2013/06/30 09:47:41, ana.balica wrote:
> MoinMoin/util/subscriptions.py:27: raise TypeError("item should be an instance
> of MoinMoin.items.Item")
> To tell explicitly the user what kind of object the function is expecting, in
> case something else is passed than Item.
> Same idea below.
In Python we use duck-typing. So please explain why this check is needed.
Sign in to reply to this message.
On 2013/07/01 21:15:33, waldi wrote: > In Python we use duck-typing. So please explain why this check is needed. Ok. Now i see what you are trying to say. If we will be having some other objects that will act like item or passing any other iterable object instead of Results, then I can remove the check.
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:20: Subscriber = namedtuple('Subscriber', [ITEMID, NAME, EMAIL, LOCALE]) PEP8 uppercase for constants
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:20: Subscriber = namedtuple('Subscriber', [ITEMID, NAME, EMAIL, LOCALE]) you mean ITEM_ID?
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:20: Subscriber = namedtuple('Subscriber', [ITEMID, NAME, EMAIL, LOCALE]) On 2013/07/02 08:20:48, ana.balica wrote: > you mean ITEM_ID? Subscriber = should be SUBSCRIBER =
Sign in to reply to this message.
On 2013/07/02 09:26:06, ReimarBauer wrote: > https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... > File MoinMoin/util/subscriptions.py (right): > > https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... > MoinMoin/util/subscriptions.py:20: Subscriber = namedtuple('Subscriber', > [ITEMID, NAME, EMAIL, LOCALE]) > On 2013/07/02 08:20:48, ana.balica wrote: > > you mean ITEM_ID? > > Subscriber = > > should be SUBSCRIBER = Subscriber is a class.
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:43: def extract_users_info(results): results is a rather generic name, can you use one which is explaining what it is about. https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:50: users = {Subscriber(result[ITEMID], result[NAME][0], result[EMAIL], result[LOCALE]) only the first name of the list?
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:43: def extract_users_info(results): On 2013/07/02 09:33:57, ReimarBauer wrote: > results > > is a rather generic name, can you use one which is explaining what it is about. maybe user_items? user_hits? it is an iterable object returned by whoosh that contains hits. https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:50: users = {Subscriber(result[ITEMID], result[NAME][0], result[EMAIL], result[LOCALE]) On 2013/07/02 09:33:57, ReimarBauer wrote: > only the first name of the list? Yep, that question was discussed and it was decided to keep just the first name.
Sign in to reply to this message.
https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... File MoinMoin/util/subscriptions.py (right): https://codereview.appspot.com/10537043/diff/61001/MoinMoin/util/subscription... MoinMoin/util/subscriptions.py:50: users = {Subscriber(result[ITEMID], result[NAME][0], result[EMAIL], result[LOCALE]) On 2013/07/02 09:40:51, ana.balica wrote: > On 2013/07/02 09:33:57, ReimarBauer wrote: > > only the first name of the list? > > Yep, that question was discussed and it was decided to keep just the first name. > add a comment about this there. It helps remembering this in some months.
Sign in to reply to this message.
|
