semanticscuttle-devel

Re: [Semanticscuttle-devel] getBookmarks() tag problems / user session problems

[Christian W. <cw...@cw...>]

Hello Mark,


> Didn't get a reply from my previous e-mail, so I went ahead and made
> the assumption to #6 and everything else should be ok.  Forgive me on
> my poor response, just been crazy after vacation.
I didn't do anything on SemanticScuttle the last week, but didn't
forget you. I began reading Winnetou I and am at the end of Winnetou
II  :)

> Also, historically
> I've not done much unit testing, but as I'm learning through this the
> unit testing can prove quite valuable.
One test fails for me; it's

1) www_bookmarksTest::testVerifyPrivateRSSLinkExists
Number of Links in Head not correct
Failed asserting that <integer:4> matches expected <integer:5>.
/home/cweiske/Dev/semanticscuttle/cwdev/tests/www/bookmarksTest.php:102

Btw, just checking for a number of links in the head isn't the best
option. Add some xpath to count the rss links only.
Instead of
> '//ns:link'
use something like
> '//ns:link[@rel="alternate" and @type="application/rss+xml"]'

Did you also add negative tests? i.e. no private feed links when the
user doesn't have a private key or he isn't logged in? (didn't look yet)

I'm also not fully happy with the duplicated code in rss.php (the
privatekey login and check if that didn't work), but could live with it
for now.


> I think I've added what you
> requested, so let me know if you see anything else.  I'm a bit of a
> novice on xpath but was able put it together.
> 
> I've also merged with master.  :)
Seems the merge didn't fully work; the rss icon on the pager doesn't
work anymore since it tries to use the old images/rss.gif location.
Don't worry, that happens to the best :)


Btw, how did you fix the time-problematic unit tests now? Do they run
for you now?

Oh, and I'd like to see smaller commits. For example, the reversion of
the gmdate changes should be a single commit.


In www/index.php
> filter($sitename . sprintf(T_(': Recent bookmarks (private)')) .
> $currentUsername),
make the full string translatable, including the user name:
> T_(': Recent bookmarks (private) %s')
(there was a space missing after the ) btw)
and give the user name to sprintf as parameter - the sprintf was
totally useless since there were no placeholders in the string.

-- 
Regards/Mit freundlichen Grüßen
Christian Weiske

-=≡ Geeking around in the name of science since 1982 ≡=-

Re: [Semanticscuttle-devel] getBookmarks() tag problems / user session problems

[Mark P. <mpe...@gm...>]

Hey Christian,

On Tue, Jun 7, 2011 at 3:08 PM, Christian Weiske <cw...@cw...> wrote:
> Hello Mark,
>
>
>> Didn't get a reply from my previous e-mail, so I went ahead and made
>> the assumption to #6 and everything else should be ok.  Forgive me on
>> my poor response, just been crazy after vacation.
> I didn't do anything on SemanticScuttle the last week, but didn't
> forget you. I began reading Winnetou I and am at the end of Winnetou
> II  :)
Interesting.  I've never heard of the book, but it's obviously a very
popular series (selling over 200 million copies).  I may head down to
our local used book store to see if they have it.  And not that it
means much, but I'm like a fraction of Cherokee Indian.  For that
matter, I also have some German heritage too... :)

>
>> Also, historically
>> I've not done much unit testing, but as I'm learning through this the
>> unit testing can prove quite valuable.
> One test fails for me; it's
>
> 1) www_bookmarksTest::testVerifyPrivateRSSLinkExists
> Number of Links in Head not correct
> Failed asserting that <integer:4> matches expected <integer:5>.
> /home/cweiske/Dev/semanticscuttle/cwdev/tests/www/bookmarksTest.php:102
>
> Btw, just checking for a number of links in the head isn't the best
> option. Add some xpath to count the rss links only.
> Instead of
>> '//ns:link'
> use something like
>> '//ns:link[@rel="alternate" and @type="application/rss+xml"]'
I've updated the xpath and did some additional checking.  Forgive my
inexperience with xpath.  Still on a learning curve with it.  I'm
loving it though!  I've also added a file for index.php.

>
> Did you also add negative tests? i.e. no private feed links when the
> user doesn't have a private key or he isn't logged in? (didn't look yet)
I didn't, but I have now.

>
> I'm also not fully happy with the duplicated code in rss.php (the
> privatekey login and check if that didn't work), but could live with it
> for now.
I've looked this code over quite a bit and I could pull a couple lines
of code out to remove some of the duplication, but I think that whole
if/else section may need to be completely reworked and I don't feel
comfortable (yet) to do that.  Let's get this revision out and then we
can rework it.

>
>
>> I think I've added what you
>> requested, so let me know if you see anything else.  I'm a bit of a
>> novice on xpath but was able put it together.
>>
>> I've also merged with master.  :)
> Seems the merge didn't fully work; the rss icon on the pager doesn't
> work anymore since it tries to use the old images/rss.gif location.
> Don't worry, that happens to the best :)
Fixed!  Duh!  :)

>
>
> Btw, how did you fix the time-problematic unit tests now? Do they run
> for you now?
After reverting back to gmdate(), I think there are only two changes
that was necessary.  And yes, they run perfectly for me.  Please
verify that I've done the right thing.
1) In tests/Bookmark2TagTest.php, in testGetPopularTagsDays(), the use
of "today" doesn't return time (defaults to 00:00:00 AM) so I changed
it to now which does.
2) In Service/Bookmark2Tag.php in getPopularTags(), there was a date()
call that was never migrated over to use gmdate().

>
> Oh, and I'd like to see smaller commits. For example, the reversion of
> the gmdate changes should be a single commit.
Understood.  I completely understand the usability of smaller commits.

>
>
> In www/index.php
>> filter($sitename . sprintf(T_(': Recent bookmarks (private)')) .
>> $currentUsername),
> make the full string translatable, including the user name:
>> T_(': Recent bookmarks (private) %s')
> (there was a space missing after the ) btw)
> and give the user name to sprintf as parameter - the sprintf was
> totally useless since there were no placeholders in the string.
Updated.  Please review to make sure it is ok.

Cheers,
Mark

>
> --
> Regards/Mit freundlichen Grüßen
> Christian Weiske
>
> -=≡ Geeking around in the name of science since 1982 ≡=-
>
> ------------------------------------------------------------------------------
> EditLive Enterprise is the world's most technically advanced content
> authoring tool. Experience the power of Track Changes, Inline Image
> Editing and ensure content is compliant with Accessibility Checking.
> http://p.sf.net/sfu/ephox-dev2dev
> _______________________________________________
> Semanticscuttle-devel mailing list
> Sem...@li...
> https://lists.sourceforge.net/lists/listinfo/semanticscuttle-devel
>
>

Re: [Semanticscuttle-devel] getBookmarks() tag problems / user session problems

[Christian W. <cw...@cw...>]

Hello Mark,


> > I didn't do anything on SemanticScuttle the last week, but didn't
> > forget you. I began reading Winnetou I and am at the end of Winnetou
> > II  :)
> Interesting.  I've never heard of the book, but it's obviously a very
> popular series (selling over 200 million copies).  I may head down to
> our local used book store to see if they have it.  And not that it
> means much, but I'm like a fraction of Cherokee Indian.  For that
> matter, I also have some German heritage too... :)

Karl May, the German, is very popular here. The Winnetou indian novels
are entertaining to read - even though Karl May never has been in the
USA - and I lately asked myself why I didn't read the books when I was
a child. Since it's never too late to catch up on books, I got them
from Project Gutenberg as epub files for my nook ebook reader and am
entertained by them, even if Karl May really exaggerates here and
there :)


> > One test fails for me; it's
> > 1) www_bookmarksTest::testVerifyPrivateRSSLinkExists
> > Number of Links in Head not correct

I solved the problem: You missed a the unittestmode URL parameter.
Since I configured my SemanticScuttle to use a different database for
unit tests, the SemanticScuttle in non-unittestmode behaves differently
than in unit test mode because the data set is differently.
Now I just see that this feature is not really documented :/

Create a file
> data/config.testing.php
and put a different database in it:
> $dbname = 'semanticscuttle_tests';



> I've updated the xpath and did some additional checking.  Forgive my
> inexperience with xpath.  Still on a learning curve with it.  I'm
> loving it though!  I've also added a file for index.php.
Great.


> > I'm also not fully happy with the duplicated code in rss.php (the
> > privatekey login and check if that didn't work), but could live
> > with it for now.
> I've looked this code over quite a bit and I could pull a couple lines
> of code out to remove some of the duplication, but I think that whole
> if/else section may need to be completely reworked and I don't feel
> comfortable (yet) to do that.  Let's get this revision out and then we
> can rework it.
Yep.


> > Btw, how did you fix the time-problematic unit tests now? Do they
> > run for you now?
> After reverting back to gmdate(), I think there are only two changes
> that was necessary.  And yes, they run perfectly for me.  Please
> verify that I've done the right thing.
> 1) In tests/Bookmark2TagTest.php, in testGetPopularTagsDays(), the use
> of "today" doesn't return time (defaults to 00:00:00 AM) so I changed
> it to now which does.
> 2) In Service/Bookmark2Tag.php in getPopularTags(), there was a date()
> call that was never migrated over to use gmdate().
Great. 2 more bugs fixed :)


> > In www/index.php
> >> filter($sitename . sprintf(T_(': Recent bookmarks (private)')) .
> >> $currentUsername),
> > make the full string translatable, including the user name:
> >> T_(': Recent bookmarks (private) %s')
> > (there was a space missing after the ) btw)
> > and give the user name to sprintf as parameter - the sprintf was
> > totally useless since there were no placeholders in the string.
> Updated.  Please review to make sure it is ok.

Looks good.


-- 
Regards/Mit freundlichen Grüßen
Christian Weiske

-=≡ Geeking around in the name of science since 1982 ≡=-

Re: [Semanticscuttle-devel] getBookmarks() tag problems / user session problems

[Christian W. <cw...@cw...>]

Hello Mark,


> I think I've added what you
> requested, so let me know if you see anything else.
I did a final code review and noticed that the core functionality of
private keys - getting to see protected and private bookmarks in a
feed - does not work. Apart from that, everything is looking really
fine now.

The reason is that in your latest commit, you removed the code that set
the current user ID in Service_User::loginPrivateKey, so nobody knows
that the user has been authenticated.

Also nobody caught that because there are no unit tests for it :)
Please add them the same way you did to check if the rss links are
there (fetch URL and inspect the rss xml to see if the private and
protected (watchlist) links are in there. Also do verify that private
bookmarks of different users are not visible.

-- 
Regards/Mit freundlichen Grüßen
Christian Weiske

-=≡ Geeking around in the name of science since 1982 ≡=-

Re: [Semanticscuttle-devel] getBookmarks() tag problems / user session problems

[Mark P. <mpe...@gm...>]

Hey Christian,

I've fixed the code and added a couple simplistic tests.  I can expand
the tests more this weekend, but wanted to live up to my promise of
getting something for you tonight.  Thanks for your patience pal.

Cheers,
Mark

On Thu, Jun 9, 2011 at 2:44 AM, Christian Weiske <cw...@cw...> wrote:
> Hello Mark,
>
>
>> I think I've added what you
>> requested, so let me know if you see anything else.
> I did a final code review and noticed that the core functionality of
> private keys - getting to see protected and private bookmarks in a
> feed - does not work. Apart from that, everything is looking really
> fine now.
>
> The reason is that in your latest commit, you removed the code that set
> the current user ID in Service_User::loginPrivateKey, so nobody knows
> that the user has been authenticated.
>
> Also nobody caught that because there are no unit tests for it :)
> Please add them the same way you did to check if the rss links are
> there (fetch URL and inspect the rss xml to see if the private and
> protected (watchlist) links are in there. Also do verify that private
> bookmarks of different users are not visible.
>
> --
> Regards/Mit freundlichen Grüßen
> Christian Weiske
>
> -=≡ Geeking around in the name of science since 1982 ≡=-
>
> ------------------------------------------------------------------------------
> EditLive Enterprise is the world's most technically advanced content
> authoring tool. Experience the power of Track Changes, Inline Image
> Editing and ensure content is compliant with Accessibility Checking.
> http://p.sf.net/sfu/ephox-dev2dev
> _______________________________________________
> Semanticscuttle-devel mailing list
> Sem...@li...
> https://lists.sourceforge.net/lists/listinfo/semanticscuttle-devel
>
>

Re: [Semanticscuttle-devel] getBookmarks() tag problems / user session problems

[Christian W. <cw...@cw...>]

Hello Mark,


> I've fixed the code and added a couple simplistic tests.
You are setting the current user id now:
> $this->setCurrentUserId($row[$this->getFieldName('primary')], true);

But unfortunately, the ID gets set permanently:
> public function setCurrentUserId($user, $storeInSession = false)


-- 
Regards/Mit freundlichen Grüßen
Christian Weiske

-=≡ Geeking around in the name of science since 1982 ≡=-

Re: [Semanticscuttle-devel] getBookmarks() tag problems / user session problems

[Mark P. <mpe...@gm...>]

Cut/Paste burns me again.  I had it right initially, but cut/pasted
another line and failed to fix the argument.  Fixed.  Wonder if that
would be worthy of another unit test...  :)

Cheers,
Mark

On Wed, Jun 15, 2011 at 8:27 AM, Christian Weiske <cw...@cw...> wrote:
> Hello Mark,
>
>
>> I've fixed the code and added a couple simplistic tests.
> You are setting the current user id now:
>> $this->setCurrentUserId($row[$this->getFieldName('primary')], true);
>
> But unfortunately, the ID gets set permanently:
>> public function setCurrentUserId($user, $storeInSession = false)
>
>
> --
> Regards/Mit freundlichen Grüßen
> Christian Weiske
>
> -=≡ Geeking around in the name of science since 1982 ≡=-
>
> ------------------------------------------------------------------------------
> EditLive Enterprise is the world's most technically advanced content
> authoring tool. Experience the power of Track Changes, Inline Image
> Editing and ensure content is compliant with Accessibility Checking.
> http://p.sf.net/sfu/ephox-dev2dev
> _______________________________________________
> Semanticscuttle-devel mailing list
> Sem...@li...
> https://lists.sourceforge.net/lists/listinfo/semanticscuttle-devel
>
>

Re: [Semanticscuttle-devel] getBookmarks() tag problems / user session problems

[Christian W. <cw...@cw...>]

Hello Mark,


> Cut/Paste burns me again.  I had it right initially, but cut/pasted
> another line and failed to fix the argument.  Fixed.  Wonder if that
> would be worthy of another unit test...  :)
Definitely. We've been talking about that special problem for weeks.

-- 
Regards/Mit freundlichen Grüßen
Christian Weiske

-=≡ Geeking around in the name of science since 1982 ≡=-