Suitecrm: Suggestion : grade PR by expected complexity

[horus68]

Issue

This is one idea to move into the Trello app.
This is applicable to the Salesagility way of work and thinking the way the human mind works.
After all clean up done in the issues are, moving suggestions to Trello, is now possible to have a look into the issues and PR situation. Even if you work harder the numbers will take too many time to drop.

The situation: 514 issues / 236 PR

This could be a crazy idea but there is a need to think different:

• What about create a grade on the expected complexity of a PR to be merged?
• Create an internal rule so junior staff are also able to to merge the easy PR.
• You could also relate the complexity grade on PR reviewed by "x amount" of selected community users

Why? There are days when one worker of yours have little time or is not in the mood to heavy work (by the end of the day!) but needs to fill in some time... they go to Github, filter for "easy PR" and reviews/merge one or two.
All done, the project gains one more PR merged and the PR merge average will increase!
And maybe that user will gain the mood to tackle an additional PR next day!

Context

There is also a need to make easier a PR to be merged.
In the last 8 days there was 9 PR merged (and it was a good week here!).
So... the 236 PR to be merged could take too many time.

Not counting the impact on the issues labeled "BUG" and with a "FIX proposed" too (ok.. counting 201)!
https://github.com/salesagility/SuiteCRM/issues?q=is%3Aopen+is%3Aissue+label%3A%22Fix+Proposed%22+label%3Abug

[chris001]

Agreed. At 9 PR merged per week, assuming the rate of merge keeps up to last week, which it probably wouldn't unless a fundamental treatment of PR improves, that's 6 more months more of waiting until they're all merged, that's far too slow.

[samus-aran]

Hey guys,

I was going to approach yous on the forums in regards to this this week (Seriously @horus68 you need to stop reading my mind!), so I'll include this github suggestion into it.

Basically that is exactly what we purpose to do! We are currently training a dedicated support developer to review and assess PRs currently on Github (you have probably seen them around). But on top of this we will include more senior internal developers to do the final review and complete the merge. However as numbers don't lie it will be an increase but not what we are hoping to achieve for 7.9

Regards to the community, exactly that - I was thinking of at least 2 members of the community must assess the PR and confirm that it works. A little process will needs to be put into place to ensure that community users try and mix it up abit in regards to their environments but that can be finalised fairly simply. Thought to consider is how do us at salesagility know that the two community members have reviewed the PR for us to then do the final review... I thought of mentions but if you don't know who to mention we may get normal mentions versus review mentions and get some lost... anyone know an easy way on Github?

Our bigger plan is to ideally start using Projects within Github to track current progress of certain issues and this could also apply to the Community's testing and reviewing- but again, a little thought will need to be put into how everything will fit in together.

I'll move this to Trello once I have created the forum post later today!

[pgorod]

Here's a list of possible searches concerning Review status:

• Filter pull requests that haven't been reviewed yet: state:open type:pr review:none
• Filter pull requests that require a review before they can be merged: state:open type:pr review:required
• Filter pull requests that a reviewer has approved: state:open type:pr review:approved
• Filter pull requests in which a reviewer has asked for changes: state:open type:pr review:changes_requested
• Filter pull requests by reviewer: state:open type:pr reviewed-by:octocat
• Filter pull requests by requested reviewer: state:open type:pr review-requested:octocat

Maybe if SalesAgility employees regularly search for "review:approved" they could catch the community reviews that have advanced since last time they checked.

It would be nice to be able to search by "reviewed by two people", but I can't find a way to do that...

[pgorod]

But a better suggestion, one that I've been meaning to do for quite a while, is for SalesAgility to promote some members of the community. Just get some criteria (number of merged PR's, work in the forums, civility dealing with others, ask us to send in our CV's, whatever) and then promote a few people to be able to add tags, or even merge simple PR's that you assign to them. You can always revert.

By the way, this should already be happening (much lower risks) for the Documentation on the wiki. The current closed wiki is an antithesis of what a wiki is supposed to work like, and it is even contradictory to the open-source culture of SA (which I thing is very genuine and deeply ingrained).

[chris001]

This is a wicked app which connects to github and seriously helps organize categorize and get a better overview when you have a huge number of Suggestions Issues and PRs. Free unlimited license for open source projects. Hosting is available for free or self host. Definitely recommend taking a look at it. https://github.com/integrations/youtrack

[samus-aran]

Thanks @pgorod and @chris001

That is something we certainly take into consideration @pgorod - Our goal is to get the processes down for efficiency internally as a priority and then introduce more members into the mix i.e advocate (dedicated is the wrong word as you are all dedicated) community members. Obviously that will take time but I hope everyone does see we have been (tho very recently) really trying to tackle this with the assistance of the community. :+1:

@chris001 We currently use that internally and I personally am not the greatest fan of it. However, perhaps that is something I need to resolve.

[samus-aran]

Hi Everyone,

Obviously I wasn't able to post something up yesterday. I will continue to plan this out and should get a fuller scope of this out by this week (In time for Easter holidays!).

[chris001]

@samus-aran
I'm in favor of:

1. Always auto-merge PRs with no activity in 2 weeks or more.
2. Get all the tests from Sugar 6.5 up and running - this shouldn't take much and would provide powerful protection against regressions. This requires replacing all uses of die to sugar_die, modify sugar_die to throw an exception. This has already been discusses and is understood to be correct.
3. Get fully working: Travis, PHP-CS-Checker, Scrutinizer, CodeCoverage Hub, etc, all working fully to test the code on all versions of PHP 5.3 - 7.1. These are extremely important automatic tools for lightening the burden and saving enormous amount of time otherwise spent on painstaking manual human reviews.

[samus-aran]

Hi Guys.( @horus68, @pgorod, @gunnicom and chris, but I've already tagged you in)

Posted on the forums regarding a more detailed scope of Community Reviewing and testing. We can move the discussion there and create this within the suggestion box and move it to 'Design' column.

https://suitecrm.com/forum/suggestion-box/13863-community-reviewing-process-discussion

@chris001 - Really? Auto-merge always? Are we talking about scenarios that the provider of the fix also includes unit testing for said fix? Because its the only way I can see that happening that the other automatic testing doesn't provide. Would be glad to hear other projects that do that auto merging after two weeks scenario to see what they have in place before hand (probably what you listed afterwards but still would be good to know).

[samus-aran]

Suggestion : grade PR by expected complexity

[chris001]

@samus-aran

1. For background on why to auto-merge after (you name it, any condition is satisfied, 2 or more people give the thumbs up, or X days has passed with no objection or no activity of any kind on the PR's discussion thread), see this discussion: https://github.com/isaacs/github/issues/543
2. The idea is, any Merge can be Un-Merged, it doesn't hurt anything, it's not as if you're dismantling a bomb.
3. Furthermore, as soon as composer update becomes working for the main app source code (and that day cannot come soon enough, the PR is available and should be merged asap), any instance of SuiteCRM in the field can check every day for updates, and obtain critical code fixes published the night before, in under a minute, which is how it should be, similar to Windows update, linux package updates, yada yada.
4. Automating more of how you deal with PRs is a perfect opportunity as well, to require phpunit tests for any PR. They're easy as hell to write, and many IDEs have support for phpunit built in, to help you write them, and run them, and find flaws before you push your PR code to github. So requiring phpunit tests with PRs, should be a no-brainer.
5. This is also a perfect opportunity for @salesagility to setup and take advantage of all the mainstream automatic code checking tools. CircleCI, ScrutinizerCI, Travis-CI, CodeCoverage Hub, PHP-CS-Checker. Why make life harder for yourself, SA. These tools are free, they run on github, and they're standardized. Please use them. Some example projects that use them quite effectively: privacyidea (a tiny yet excellently automated python project), owncloud (this latter one is a huge PHP project and is a shining example to look to and emulate).