Bug in QnByArchitect.understand(...)

[amihaiemil]

In method QnByArchitect.understand(...), the decision wether rultor understands the request or asks the architect to confirm is based on boolean legal which is determined like this:

final boolean legal = logins.isEmpty()
            || logins.contains(
                comment.author().login().toLowerCase(Locale.ENGLISH)
            )
            || logins.contains(
                issue.author().login().toLowerCase(Locale.ENGLISH)
            );

where logins (btw, I would name it architects), contains the architects' logins.

This means that if an architect makes a PR (maybe he wants to have it in history of merges and not make the commit directly on master) or any other kind of issue, I can go and give commands to rultor and it will process them.

This is because the conditions above translate to false, false, true - there are architects declared for the project; the comment author is not an architect, but the issue author is an architect.

I tested this with the following unit test, which fails:

    @Test
    public void rejectsIfNotArchitect() throws Exception {
        final MkStorage storage = new MkStorage.Synced(new MkStorage.InFile());
        final Repo repoJeff = new MkGithub(storage, "jeff").randomRepo();
        final Issue issue = repoJeff.issues().create("", "");

        final Github amihaiemil = new MkGithub(storage, "amihaiemil");
        Repo repoMihai = amihaiemil.repos().get(repoJeff.coordinates());
        Issue issueMihai = repoMihai.issues().get(issue.number());
        final Comment.Smart mihaiComment = new Comment.Smart(
            issueMihai.comments().post("deploy")
        );

        final Question question = Mockito.mock(Question.class);
        final URI home = new URI("#");
        new QnByArchitect(
            new Profile.Fixed(
                new XMLDocument("<p><entry key='a'>jeff</entry></p>")
            ),
            "/p/entry[@key='a']/text()", question
        ).understand(mihaiComment, home);
        Mockito.verify(question, Mockito.never()).understand(mihaiComment, home);
        MatcherAssert.assertThat(
            issue.comments().iterate(),
            Matchers.<Comment>iterableWithSize(2)
        );
    }

Notice that Jeff is both issue author and project architect and amihaiemil gives the command to rultor.

Fix:
remove the condition

logins.contains(
    issue.author().login().toLowerCase(Locale.ENGLISH)
);

After the bug is fixed it would also be nice to add this unit test to QnByArchitectTest

[amihaiemil]

@alex-palevsky I think this issue is pretty urgent.

[amihaiemil]

@alex-palevsky @original-brownbear I made PR #1106 for this issue.

[amihaiemil]

@alex-palevsky ping

[original-brownbear]

@amihaiemil I'm aware of the PR/Issue, will review shortly. Sorry many things going on right now, but this is on the radar for today for me :)

[amihaiemil]

@original-brownbear sure, thanks : )

[original-brownbear]

@amihaiemil I think this is the intended behaviour but I may be misunderstanding the issue.

From what I understand you criticise the face that this happens:

1. Architect makes PR.
2. Commander ( or is it arbitrary person and not commander ) calls merge
3. Rultor merges without confirmation.

I think if its commanders only in step two this is not a bug right ? At least I did always take this for the intended behaviour. Or do you see an issue with this ?

[amihaiemil]

@original-brownbear If you test the method in isolation (see unit test), then it can be anyone in step 2, not just commanders. I dont know the whole code yet, maybe there's a layer on top of it which only lets commanders talk to rultor. But even if it lets just commanders, it looks a bit inconsistent to me, since the same commander, with the same command needs approval on other issues that are not authored by the architect. We can test this easily if you make a small pr, i would say "merge" and see how it acts.

[amihaiemil]

@original-brownbear as you can see, in PR #1107 I told rultor to merge and he asked you to confirm it, which means the method here was called with my Profile; If you were the PRs author it would have merged, exactly like the unit test.

[amihaiemil]

@original-brownbear it's also not called somewhere else in the code. Simply search the repo for the string "QnByArchitect.denied" (the key of the message from phrases_en_US.properties )and you will see it's only used in the method in cause.

[amihaiemil]

@original-brownbear from my point of view this bug is confirmed :)

[original-brownbear]

@amihaiemil yea you're right. Also this at least extends to the stop command as I could now observe in a different project.
Sorry for the delays with me btw, normal operations on my end will resume within 12-16h!

[original-brownbear]

@alex-palevsky valid bug.

[amihaiemil]

@original-brownbear Can this also be assigned to me since I made a PR with the fix? (I'm not sure if I can be both author and resolver)

[alex-palevsky]

@alex-palevsky valid bug.

@original-brownbear added "bug" tag to this issue

[amihaiemil]

@alex-palevsky assign me here please.

[alex-palevsky]

@amihaiemil I set the milestone to 2.0 since there is nothing set yet

[alex-palevsky]

@amihaiemil thanks for tis bug, I topped your account for 15 mins, transaction AP-28280969RL475321G

[amihaiemil]

@alex-palevsky assign me here please.

[alex-palevsky]

@alex-palevsky @original-brownbear I made PR #1106 for this issue.

@amihaiemil thanks

[amihaiemil]

@original-brownbear can I have this assigned to me?

[original-brownbear]

@amihaiemil let me find out, you're not on the Rultor team so I need to ask first :) see below in a sec.

[original-brownbear]

@yegor256 can I assign @amihaiemil here, despite him not being on the team? Seems he did some work towards fixing this already.

[amihaiemil]

@yegor256 can you have a look here? Thanks

[amihaiemil]

@yegor256 ping

[amihaiemil]

@yegor256 ping

[yegor256]

@original-brownbear yes, you can, but keep in mind that it's a bad (very bad) practice to have the same person as a bug reporter and a bug fixer

[original-brownbear]

@yegor256 yup, well see below anyhow.

[original-brownbear]

@amihaiemil sorry not really as a result of @yegor256 's comment above alone, but more the fact that the PR does not fully resolve the issue ( and introduce a new one, will put the effort of explaining why that is so later ) + CR has not been assigned yet I'd rather postpone this one in favour of more pressing EC2 ones.

[original-brownbear]

@alex-palevsky this is postponed.

[amihaiemil]

@original-brownbear sure, no worries

[alex-palevsky]

@alex-palevsky this is postponed.

@original-brownbear thanks, I added "postponed" label

[alex-palevsky]

@alex-palevsky this is postponed.

@original-brownbear got it, someone else will be assigned soon