Broken URL on game move

[sphaso]

Hello! I've just encountered a strange occurence. Steps:

• go to this game (I wasn't logged in, if it makes any difference): https://gokibitz.com/kifu/VkXK5jd7
• go to the next move (either using display arrows or keyboard arrows)
• around move 18 or 19 the URL instead of being ../VkXK5jd7?path=19 becomes https://gokibitz.com/?path=19
• this results in a redirect to the homepage

I'm not familiar with the codebase. At first I thought you were doing some kind of media discovery to go to the next move, then I realized that you only fetch comments move by move, so you simply iterate through the SGF that you load when you access the game page the first time. I guess there's some weird corner case when you build the URI for the next move?

Thank you and good luck!

EDIT: Ok so... diving into the code a bit. I see that $scope.player is from a transcluded directive (sgf.js) which wraps wGo. Something tells me the issue is not upstream, why would wGo change the URL?!

EDIT2: I've noticed something interesting! Go to this game: https://gokibitz.com/kifu/4JHZE2om I'm redirected at the very first move, however! If I go back and remove the "?path=xx" from the URL I can go on watching the game and the "?path" part doesn't reappear! Maybe there's a problem in these lines? (controllers/kifu.js ln 116) `` // Put the move in the query string for super sweet permalinks

$location.search('path', pathFilter($scope.kifu.path, 'string')); ``

[levelonedev]

Hi Sphaso!

We are definitely the ones changing the URL. It's to keep it up to date and make it easier for people to share specific moves.

Thanks for documenting it with a reproducible test case!

If you feel like fixing it up yourself you are in the correct area. I think it must be the path filter messing up. If not someone will be sure to take a look at it soon since it should be simple.

Christopher

Christopher

[sphaso]

I'd love to tackle it myself, thanks! I tried setting up a dev env on my Arch box but I'm having a bit of an issue. It seems that the local website is not bringing up any .js nor .css besides app.js. I didn't touch the gulp file. I'm a bit inexperienced with the whole nodeJS toolchain, do you have any idea on how to troubleshoot this?

[levelonedev]

Great! I set up on an Ubuntu box and Nate uses OS X. Can you show some pictures or output? I am new to nodejs as well but I can hopefully figure it out as I have hit a lot of the issues.

[levelonedev]

And in case you didn't notice the instructions for development set up were just changed slightly with pull request #88. It might help!

[neagle]

Perfect reproduction steps. This is very strange! I was able to reproduce the issue and will check it out tonight. Thanks for taking the effort to report this.

Nate Eagle

On May 16, 2015, at 4:38 AM, sphaso notifications@github.com wrote:

Hello! I've just encountered a strange occurence. Steps:

go to this game (I wasn't logged in, if it makes any difference): https://gokibitz.com/kifu/VkXK5jd7 move to the next move (either using display arrows or keyboard arrows) around move 18 or 19 the URL instead of being ../VkXK5jd7?path=19 becomes https://gokibitz.com/?path=19 this results in a redirect to the homepage I'm not familiar with the codebase. At first I thought you were doing some kind of media discovery to go to the next move, then I realized that you only fetch comments move by move, so you simply iterate through the SGF that you load when you access the game page the first time. I guess there's some weird corner case when you build the URI for the next move?

Thank you and good luck!

— Reply to this email directly or view it on GitHub.

[sphaso]

@levelonedev actually, it seems only the CSS is missing. With some trouble (due to the sketchy layout) I was able to register and upload a game, so I'm guessing all the functionalities are there on both client and server side. Not sure what screenshot \ ouput might give clues... gulp watch output? I've also noticed there's no map file, zo... no live debugging? good old console.log?

@neagle is it cool if I try to fix it? consider it a sort of initiation ritual :P but if you think it's urgent please don't mind me.

[neagle]

Make sure you've run gulp default (or just gulp) if it seems like some of the files are missing.

Yeah, I'd love to have you give it a shot! Thank you. :D

Nate Eagle

On May 16, 2015, at 2:11 PM, sphaso notifications@github.com wrote:

@levelonedev actually, it seems only the CSS is missing. With some trouble (due to the sketchy layout) I was able to register and upload a game, so I'm guessing all the functionalities are there on both client and server side. Not sure what screenshot \ ouput might give clues... gulp watch output? I've also noticed there's no map file, zo... no live debugging? good old console.log?

@neagle is it cool if I try to fix it? consider it a sort of initiation ritual :P but if you think it's urgent please don't mind me.

— Reply to this email directly or view it on GitHub.

[sphaso]

News from the trenches:

• the problem with the CSS was that "npm install" had skipped some modules (namely "require" and "stream-browserify") giving a warning I didn't notice the first time
• the bug seems to be strictly related to not being logged in. I cannot reproduce when logged in
• further proof: when the redirect occures I get a 401 (unauthorized) on /api/markdown, which makes me think pathFilter might be a red herring
• my current hypothesis is that, quite simply, /api/markdown needs to be accessed anonimously

I'll keep digging.

[sphaso]

Well, it seems pretty straightforward now. The "commentBox" directive at line 43 calls /api/markdown

$http.post('/api/markdown/', {
                        markdown: element.val()
                    }, {
                        timeout: canceler.promise
                    })
                        .success(function (data) {
                            $scope.preview = data.markup;
                        });

However, this route is protected (server/routes/markdown line 11):

router.post('/', auth.ensureAuthenticated, function (req, res) {
    var html = marked(parseLabels(req.body.markdown)) || '';
    res.json({
        markup: html
    });
});

Is it safe to remove the authentication on this route? Was there a reason why it was put? It seems that this api simply translates markdown into HTML. I'm not much of an api designer, but shouldn't it be a GET?

[levelonedev]

Nope, I don't think authentication is needed. Thanks for digging down to find this! Remove it and test localhost just to be sure and then submit a pull request!

[neagle]

Hm, I think we need to peek a bit deeper. The comment box shouldn't be calling the markdown API when you're not logged in: you shouldn't even see the comment box if you're not logged in. I put it behind auth because I didn't want to make it available as an anonymous markdown service, even if such abuse is pretty unlikely. I think we need to figure out the root cause, which is what's causing a call to get sent in this case where the user's not logged in.

Thanks for investigating this and big thanks to @levelonedev for helping get you ramped up this afternoon. You have no idea how happy I was to see this comment thread happen. :dancers:

[neagle]

Ah ha: so the call that's getting triggered has the text from the game comment, in this case: "This was probably too greedy. I think the honte play is simply K8, planning to use the thickness to reduce the right later. White should remember that between the lower left shimari and the top side, it's still an even game. There's no need to rush."

I only recently added parsing of the text in the game comment, so it's a good candidate to have a newly introduced bug.

[sphaso]

@neagle @levelonedev Thank you guys! I had fun and hopefully I'll have more :)

[sphaso]

So I'm guessing we don't want to show the commentBox when the user is not logged in? (i.e. add a condition here)

div.panel.panel-default(
                ng-if='sgfComment || kifu.owner.username === currentUser.username'
            )

I'm not exactly sure if this will be enough to stop from calling the API. Where is the 'gkCommentBox' directive injected?

[neagle]

We already don't show the editGameComment box to non-logged-in users, since it doesn't show for anybody but the owner.

That textarea uses the gkCommentBox directive, as you note, but it doesn't include a preview element, since we're not actually parsing markdown for game comments (yet?). Some condition, however, is making preview get fired even though it doesn't seem obvious than it should. As I see it, the fix is:

1. Figuring out why it's getting called, because we at least should know, right? :smile:
2. Amending the conditional that fires preview to make sure it only fires when appropriate, and/or fixing some more mysterious cause, depending on the results of #1.

[sphaso]

Oh ok, sorry, I forgot to grep for the snake-case of the directive :) gets me everytime! Things are much clearer now, thanks.

div(ng-show='editGameComment')

This div will get rendered even if you're not the owner (in fact, even if you're not logged in). This means that the textArea is bound to the directive, just hidden via CSS.

Replacing that ng-show for an ng-if will probably do the trick, like Christmas morning! Alternatively, I wonder if you can make the above div a child of the ng-if'ed div (kifu.jade)

div(ng-if='!sgfComment && kifu.owner.username !== currentUser.username && !editGameComment')  

I'll confirm \ disprove my hypothesis as soon as I get home from work.

[neagle]

It seems like you already have some Angular experience: is that true? Or are you just picking up details about how it works astonishingly quickly?

Good research on this. I think you're almost certainly right that changing div(ng-show='editGameComment') to div(ng-if='editGameComment') would fix the problem, but to me that feels like ignoring the problem's root cause, which is that the directive's not being smart about when to fetch a preview.

I hope you'll forgive me, I didn't do this deliberately to usurp your task, but since I needed to refactor gkCommentBox for something @levelonedev is doing anyway, I slipped in a fix for this. Basically, I'm checking whether a preview value has actually been specified before doing any of the logic for fetching a preview, which is a performance improvement, anyway.

Btw, you also inspired me to get lint / code style checkers in our gulp setup so that it's easier for other devs to jump in, as well as to get us set up with Travis CI so that new builds and pull requests are automatically tested. (Well, our tests right now consist solely of lint/code-style checks, but we'll improve that soon.) Thanks again for your help!

[sphaso]

Hey @neagle no problem! I have about a year of Angular experience, I'm not an ng-guru but I get by, thanks :) I agree that simply putting ng-if is a bit of an hack, I debated this in my head for a while. I think that ultimately it's not that bad, the problem is that the directive shouldn't be injected at all. You could put the whole HTML in a partial view and put this logic (don't show for users who are not logged in, don't own the game etc.) in the template controller so the behavior becomes more understandable (which, from what I see, is why the whole ng-if idea smells.) We should also consider trade-offs: ng-if won't render the HTML, so when a user actually can and want to edit a comment, the template will have to be rendered, linked etc. At the same time, leaving an ng-show will keep the directive bound and all the bindings will fire at any $scope change. But it's probably not worth thinking about all this for now :) Great news about Travis and gulp, can't wait for proper unit tests :) I'm very green on angular unit testing (the TDD church will banish me) so it will be fun to write some!

[neagle]

I'm not an ng-guru, either, though I'm starting to get more comfortable. GoKibitz is my first and only significant project in Angular, though I have a couple smaller projects at work that I've used Angular for. In general, I'm pretty happy with it: even though there are lots of things that could use refactoring since I know more now than I did when I originally implemented them, having a framework like Angular makes changes and new features a lot easier than it might be otherwise.

Proper unit tests will be great: I'm new to the TDD church, too, and most of my experience is with relatively simple mocha tests for NPM modules. But one of the main benefits of GoKibitz for me is the chance to grow as a developer, and if it helps some of the devs who contribute, too, then that's better still. :smile:

Thanks again for your help!