Update subscription API

Use fqname(s) as parameters to methods `User.subscribe()`, `User.unsubscribe()`, `User.is_subscribed_to()`.

Update methods, tests and method usages.

[Thomas.J.Waldmann, 2014-02-20 14:35:08]

please check those unrelated(?) changes.

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py
File MoinMoin/_tests/test_user.py (right):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py#n...
MoinMoin/_tests/test_user.py:142: the_user.subscribe(CompositeName(u"", NAME,
u"SomeOtherPageName"))
btw, what is this line above good for?

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py#n...
MoinMoin/_tests/test_user.py:149: (meta[NAMESPACE], NAME, pagename),
maybe you should rather search for just a part of the pagename, so one sees
whether NAME is working differently from NAME_EXACT

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py#n...
MoinMoin/_tests/test_user.py:155: the_user.subscribe(fqname)
before that, maybe check that the user is not already subscribed

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py#n...
MoinMoin/_tests/test_user.py:158: assert not the_user.is_subscribed_to([fqname])
hmm, i am asking myself whether the subscribe action should get the same fqname
as the is_subscribed_to check.

for an id, this makes sense, but for a regex, it does not, right?

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (left):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:987: abort(403)
how is removing this related to your other changes?

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:990: item=item,
?

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:1904: abort(403)
same here

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:1922: item=item,
and here

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:1156:
@frontend.route('/+subscribe/<itemname:item_name>')
will this work with generic fqname then also?

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:1168: fqname =
CompositeName(meta.get(NAMESPACE), ITEMID, meta.get(ITEMID))
.get() will give you None if the key is not there.

a) can this happen?
b) if that happens, will you code be able to deal with the None?

https://codereview.appspot.com/65070043/diff/1/MoinMoin/items/__init__.py
File MoinMoin/items/__init__.py (left):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/items/__init__.py#old...
MoinMoin/items/__init__.py:793: item=self,
and here

https://codereview.appspot.com/65070043/diff/1/MoinMoin/items/__init__.py#old...
MoinMoin/items/__init__.py:866: item=self,
here

https://codereview.appspot.com/65070043/diff/1/MoinMoin/items/__init__.py#old...
MoinMoin/items/__init__.py:918: item=self,
here

https://codereview.appspot.com/65070043/diff/1/MoinMoin/user.py
File MoinMoin/user.py (right):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/user.py#newcode570
MoinMoin/user.py:570: """ Check if user is subscribed to items defined by
fqnames
identified by

https://codereview.appspot.com/65070043/diff/1/MoinMoin/user.py#newcode574
MoinMoin/user.py:574: :returns: if user is subscribed to any of the items
described by fqnames
identified by

https://codereview.appspot.com/65070043/diff/1/MoinMoin/user.py#newcode584
MoinMoin/user.py:584: if fqname.field not in [NAMEPREFIX, NAMERE, ]:
else?

not sure about this, but maybe exactly one item's latest revision should be
really (uniquely) identified by one fqname.

like ITEMID, NAME identifies one specific item (and we use the current
revision).

if you allow non-unique fields (like TAGS, NAMEPREFIX, ...), you'll potentially
get more than one item.

https://codereview.appspot.com/65070043/diff/1/MoinMoin/user.py#newcode604
MoinMoin/user.py:604: return True
maybe add some comment to these 6 lines about what usecase they cover.

https://codereview.appspot.com/65070043/diff/1/MoinMoin/user.py#newcode617
MoinMoin/user.py:617: :       :param fqname: composite name that contains
namespace, field name and value
somehow this doesn't match.

a fqname is (in the unique ids cases) good for identifying an item.
or (in the not unique id case) good for identifying some items.

but for subscribing, this is a bit different, it is not just about field and
value, you also have some mode about how the value gets processed (see
NAMEPREFIX vs. NAMERE vs. NAME), so it doesn't look like a "fqname" here.

so this method also should not have "subscribe to a wiki page" as comment any
more, because it is quite more powerful.

https://codereview.appspot.com/65070043/diff/1/MoinMoin/user.py#newcode637
MoinMoin/user.py:637: :param fqname: composite name that contains namespace,
field name and value
see above

[ana.balica, 2014-02-23 19:23:59]

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py
File MoinMoin/_tests/test_user.py (right):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py#n...
MoinMoin/_tests/test_user.py:158: assert not the_user.is_subscribed_to([fqname])
On 2014/02/20 14:35:09, Thomas.J.Waldmann wrote:
> for an id, this makes sense, but for a regex, it does not, right?

Maybe it doesn't really make sense, though is_subscribed_to is able to handle
such fqnames. As an alternative we can vary the subscriptions, but always pass
to is_subscribed_to the fqname identified by itemid? 

But that means that we leave some paths of is_subscribed_to() untested. Maybe we
shouldn't allow passing an fqname to is_subscribed_to() at all?

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (left):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:987: abort(403)
On 2014/02/20 14:35:09, Thomas.J.Waldmann wrote:
> how is removing this related to your other changes?

This is an answer to all changes that involve removing of the item object and
its passing to the templates:
Those changes were introduced in this commit by me -
http://hg.moinmo.in/moin/2.0/rev/a704bbeb189c
Method `is_subscribed_to()` was the only method that required an item object.
Therefore I backtraced the changes and reverted to its original state.

The problem is that item object might have been used for other purposes in the
template, but I don't know how to identify it. I did manual testing, but I am
sure it can't cover everything.

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (right):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:1168: fqname =
CompositeName(meta.get(NAMESPACE), ITEMID, meta.get(ITEMID))
You are right. This can't happen, because the view is aborted if an item can't
be created.

https://codereview.appspot.com/65070043/diff/1/MoinMoin/user.py
File MoinMoin/user.py (right):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/user.py#newcode584
MoinMoin/user.py:584: if fqname.field not in [NAMEPREFIX, NAMERE, ]:
On 2014/02/20 14:35:09, Thomas.J.Waldmann wrote:
> else?

Else rev is None and we can't update subscription list based on the revision
with any other information besides a subscription string formed by NAMERE or
NAMEPREFIX. Since revision is not used later, I skipped the else part.


> not sure about this, but maybe exactly one item's latest revision should be
> really (uniquely) identified by one fqname.
> 
> like ITEMID, NAME identifies one specific item (and we use the current
> revision).
> 
> if you allow non-unique fields (like TAGS, NAMEPREFIX, ...), you'll
potentially
> get more than one item.

I thought that was the whole point of it - to be able to subscribe to more items
at the same time. The following method will simply return a bool value and so
far is used to check a single item against the list of user subscriptions.

https://codereview.appspot.com/65070043/diff/1/MoinMoin/user.py#newcode617
MoinMoin/user.py:617: :       :param fqname: composite name that contains
namespace, field name and value
On 2014/02/20 14:35:09, Thomas.J.Waldmann wrote:

> but for subscribing, this is a bit different, it is not just about field and
> value, you also have some mode about how the value gets processed (see
> NAMEPREFIX vs. NAMERE vs. NAME), so it doesn't look like a "fqname" here.

Initially I had the concerns about the variable name, because it didn't seems to
be an fqname. I would say it is a subscription described by a composite name.
composite_subscription?

[ana.balica, 2014-03-06 16:40:17]

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py
File MoinMoin/_tests/test_user.py (right):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py#n...
MoinMoin/_tests/test_user.py:149: (meta[NAMESPACE], NAME, pagename),
On 2014/02/20 14:35:09, Thomas.J.Waldmann wrote:
> maybe you should rather search for just a part of the pagename, so one sees
> whether NAME is working differently from NAME_EXACT

Could you please elaborate on that?
I am trying to understand the semantic difference between NAME and NAME_EXACT.
NAME_EXACT is returned by split_fqname(), but it's value seems to be same as
NAME.

[Thomas.J.Waldmann, 2014-03-06 16:53:13]

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py
File MoinMoin/_tests/test_user.py (right):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py#n...
MoinMoin/_tests/test_user.py:149: (meta[NAMESPACE], NAME, pagename),
the reason why we have 2 fields (NAME and NAME_EXACT) for the item name(s) is
that one can search for some pieces of the name (via NAME) and also for the
exact full name (via NAME_EXACT).

Thus, the best way to test, is to give a full exact name to test against
NAME_EXACT and only give a part of the name to test against NAME.

https://codereview.appspot.com/65070043/diff/1/MoinMoin/_tests/test_user.py#n...
MoinMoin/_tests/test_user.py:158: assert not the_user.is_subscribed_to([fqname])
for checking is_subscribed_to, you need to have an item.
to have an item, you need either to already have the item instance or a unique
identifier to fetch it yourself from storage.

for subscribing, the requirement is to get any supported / valid subscription
pattern, which differs in types from any supported / valid fqname.

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.py
File MoinMoin/apps/frontend/views.py (left):

https://codereview.appspot.com/65070043/diff/1/MoinMoin/apps/frontend/views.p...
MoinMoin/apps/frontend/views.py:987: abort(403)
> Method `is_subscribed_to()` was the only method that required an item object.

ok, if that is the only reason why it is passed to the template, this change is
a good cleanup.

> The problem is that item object might have been
> used for other purposes in the
> template, but I don't know how to identify it.
> I did manual testing, but I am
> sure it can't cover everything.

well, you see whether "item" is used in the template html.