search

monitorjbl / pr-harmony

Extra pull request workflows for Stash
GNU General Public License v3.0
33 stars 17 forks source link

Empty user list on creating PR #31

Closed bjmillar closed 8 years ago

bjmillar commented 8 years ago

We would really like to use this plugin but we are having some issues. Our setup:

I have tried all combinations of project and repository settings for PR Harmony but no matter what, the reviewer list is empty when creating a PR. I have noted other similar issues (now fixed) as we have non-lowercase repositories and users in Bitbucket. Everything else works correctly, including the merge check for the required reviewers!

Some things to note:

On the PR create screen the browser log reports "Fetching required users" but not "Loaded required users".

I tried with all sorts of combination of lowercase projects, repositories and users but to no avail.

Regards, Barry.

monitorjbl commented 8 years ago

Interesting, I haven't seen this before. Let me take a look at your examples and see if I can replicate them.

bjmillar commented 8 years ago

I am in the middle of trying to migrate our projects/engineers to Bitbucket/Git so I am happy to help if you have any debug or RCs you want to try out.

bjmillar commented 8 years ago

I blanked all the settings in the PR Harmony config pages for my repository and for the project so that it was effectively unused and now I get blank pages when attempting to open the PR Harmony config pages of the project and the repository.

intiocean commented 8 years ago

I'm seeing a similar issue where no reviewers are filled in.

Same versions as above but I don't see anything logged to the browser console.

image

monitorjbl commented 8 years ago

@intiocean Are you setting the Default Reviewers field in the configuration? Required reviewers are not automatically added to all PRs.

monitorjbl commented 8 years ago

@bjmillar I'm not able to replicate this locally. I am able to confirm case-sensitivity matters, but the plugin ought to use the case that your repos have. What is the actual value for your project key and your repo slug?

For reference, this is what I see with those repo names:

image

And here's my config:

image

Note that the "Default users" is set. This is what populates the PR on creation. You mentioned that you're not seeing the "Loaded required users", which is kinda odd. If you're not seeing any errors in your console, it could be a result of a misconfiguration on the server. A user did see issues when using HTTPS, he didn't have the server name value set properly on the Stash side so the plugin tried to communicate over HTTP and the browser blocked it with a warning.

Based on the URLs you posted, I don't think that's the case. If you're not seeing anything obviously odd, would you mind pasting a screenshot of your browser's network and Javascript console?

Wilfred commented 8 years ago

I'm consistently encountering this bug.

I set the default reviewers:

pr_harmony_settings

However, when I create a pull request, no reviewers are added:

pr_harmony_reviewers

There are no HTTP error codes in the network logs:

pr_harmony_network

and there are no errors in the browser console either:

pr_harmony_console

monitorjbl commented 8 years ago

Thanks for documenting this so clearly. I do see one odd thing in the console log. The "HERE I AM" line is a debug line I accidentally left in on the last release. It shows up in its own Javascript file on my environment but it's showing up in a batch.js file in yours. I wonder if whatever setting causes that to happen is interfering with the Javascript hook.

monitorjbl commented 8 years ago

I added a lot of extra debug output in the latest code which should hopefully help pinpoint what's going on in your environments that's causing this problem. If you have a few minutes, I've posted a build that you can download and install manually here. Once you've got it installed, you should be able to open up your browser's dev console and see a bunch of lines that look like this:

image

If you could post the entire output into this issue, it would be a huge help to figuring out what's causing this bug for some users.

bjmillar commented 8 years ago

Thanks for the debug version. Sorry I didn't get back to you yesterday. I have attached the results of some of my tests. It looks like it is a user "type" issue. We have some local users for admin/CI purposes. However the majority of our actual users login using "JIRA Server". From the various tests attached you can see that a "JIRA Server" user cannot amend the PR Harmony config - nothing is rendered. Also when at least one "JIRA Server" user is added to the list of reviewers, the reviewer list is empty when creating a PR. When I use entirely local users it all works fine.

Let me know if you need me to throw in some more debug or try anything different.

prh-debug.txt

bjmillar commented 8 years ago

Oops. Just noted that the "Result" of "TEST 5" in my attachment above states "Nothing is rendered". This is incorrect as the configuration was correctly rendered and populated.

monitorjbl commented 8 years ago

Ah, we don't use Jira as a user repository, so I guess that's the difference. I'll see if I can reproduce this with the SDK

Wilfred commented 8 years ago

Interesting that it might be related to the user type. On our test system, users log in with LDAP accounts.

bjmillar commented 8 years ago

Ours do too, but via JIRA as Bitbucket uses it as a user directory.

monitorjbl commented 8 years ago

@Wilfred Would you be able to try out the version with debug logging?

bjmillar commented 8 years ago

Yes I can.

bjmillar commented 8 years ago

Might have misunderstood. Would you like me to run Bitbucket with debug or is there a setting to enable debug for the plugin?

monitorjbl commented 8 years ago

@bjmillar Thanks but you've already given me your debug output. I was asking @Wilfred if he could send me his as well. We use LDAP but not the latest version of Bitbucket where I work, and I haven't been able to replicate this issue using the SDK yet.

monitorjbl commented 8 years ago

Atlassian accidentally broke the plugin persistence storage API from 4.0 to 4.1 so it's possible they've broken something with security in 4.1 to 4.2.

Wilfred commented 8 years ago

OK, I've installed the 2.2 snapshot and it seems to be working! Bizarre. Downgrading to 2.1 makes the issue reappear.

@monitorjbl would you still like the debug logging?

Wilfred commented 8 years ago

OK, I suspect I can see the issue.

c6160b4f5cd64d1c1ab60ac992102a760b7eabde made the following change:

-define('suggested-reviewers', [
+define('pr-harmony-create', [

However, Atlassian's suggest reviewers plugin defines a module of the same name. We have this plugin installed, so I think its JS is overriding pr-harmony's JS.

monitorjbl commented 8 years ago

Ah, interesting! I wonder why @bjmillar still has trouble though...

bjmillar commented 8 years ago

@monitorjbl I have had another play with this to see why I am having issues. The 2 issues I suffer are:

  1. I get a blank config page for LDAP (via JIRA) users and it works for my local users.
  2. If I add any LDAP users to any of the reviewer lists in the config, they do not appear in the PR create.

I just noticed that all my LDAP users are capitalised (of the form T0123456) and all my local Bitbucket users are entirely lowercase.

Luckily, I have access to one other LDAP account. I added that LDAP user to JIRA as e.g. "Server_user". When I logged into Bitbucket using this account I got the issues above. I deleted this user from JIRA and added the LDAP user into JIRA again, this time as "server_user" (all lower case!). When I logged into Bitbucket the config page was again available to me.

I added the "server_user" to the list of required users on my test project. I then logged on as one of local users to create a PR on a branch in this project. The PR create button was disabled and a correct warning was displayed about missing required users. I added the required user and the warning was removed. The required user was not automatically added to the list of reviewers. Should it be? I repeated the test using the "server_user" as a default user. In the PR Create this user was correctly prepended to the list of reviewers.

Am I suffering because of the case of the LDAP users? I know there were changes made previously as a result of the case of users. I noticed from the code that ConfigDao still "lowercases" users in method FilterInvalidUsers.apply(). As I am getting a 403 error from the config page when using LDAP users with uppercase, is this the problem?

monitorjbl commented 8 years ago

That's certainly something I can try to reproduce. I'll give it a shot, hopefully that's the culprit. I haven't seen any evidence that the origin of the user would make any difference.

monitorjbl commented 8 years ago

Awesome, it looks like I can reproduce this now! This is my configuration:

image

And this is what I see when I try to create a PR:

image

It looks like there's an NPE being thrown. You can see this in the browser logs

image

or the server logs

image

It would appear that the user lookup service from Atlassian doesn't handle uppercases anymore. This worked previously, in fact issue #5 involved uppercases and a user confirmed the fix to work. Switching to userService.getUserByName(username) seems to work for both cases.

I published a release candidate with this fix, would be able to give it a shot and see if it works for you?

bjmillar commented 8 years ago

@monitorjbl I have tried the RC. So the Create PR option now works as expected. The default users appear in the list of reviewers and a warning is displayed if the required users are not entered in the list of reviewers. However, I can only access the PR-Harmony configuration page if I log on as a "lower case" user. If I go to the configuration page as one of my uppercase users I see nothing.

monitorjbl commented 8 years ago

Shoot, must be a similar issue. Should be easy enough to replicate and fix.

monitorjbl commented 8 years ago

Yep, it turned out to be pretty easy to fix. I put a new RC version up, if you can give that a quick try I'll go ahead and release this fix to the marketplace.

bjmillar commented 8 years ago

@monitorjbl I have tested the latest RC and the config and PR Create now works for all users. Thank you very much for your support and persistence in getting this resolved.

monitorjbl commented 8 years ago

Version 2.2.0 is now live on the Atlassian Marketplace. Thanks to everyone that helped debug this issue!

AkulBhatnagar commented 8 years ago

I feel blessed that I found this page. But there is a bad news, I could see similar issue (LDAP user) in PR Harmony current release 2.3.1 which is available at Atlassian Market Place.

I even tried 2.2-RC jar. But it failed too. https://github.com/monitorjbl/pr-harmony/releases/tag/v2.2.0-RC2

Please note, I use bitbucket 4.3.2

andyliu38 commented 7 years ago

I think I'm experiencing the same thing, however, I'm using PR Harmony 2.3.1 with bitbucket server v4.10.1.

I notice in IE11, the default reviewer group are not added at p/r creation even though a default review group is set/saved. But this issue is fine in FireFox and Chrome. IE Compatibility setting seems like is not the culprit. My username is all capitalized. And the default review group I have there is a mix of username in all upper and all lower cases.

Thought? Thanks.