Community Reviewing Process discussion!

[samus-aran]

Hi Everyone,

Would like to discuss and hopefully finalise the details of the new way that ourselves (SalesAgility) and the SuiteCRM community can assist with speeding things up with PRS and overall Github handling.

There are a few areas I would like to discuss, but essentially the main focus is how can we facility the Community to assist us with successfully merged more PRs and thus decrease that steady flow of bugs.

Now, I would like to be quite transparent in the way that we had currently handled Github issues and where you (and us) see the obvious bottlenecks.

Currently issues and Prs linking to issues are tested usually first by an internal junior member of the SalesAgility team (junior: experienced PHP developer, but low/medium knowledge of SuiteCRM architecture.). Unable to confirm the bug or fix the issue/pr is identified so with the ‘Pending Input’ label.

Once the bug has been confirmed the labelling is assigned and if the PR resolves the issue then the label ‘Assessed’ is assigned. In some cases if the Community has tested the PR (more than one community member) then the general consensus is that the bug has been resolved and the PR is assigned to ‘Assessed’ without the first line of reviewing by SA.

When Prs have been labelled with ‘Assessed’ a senior maintainer will review the code to either provide insight of a better SuiteCRM implementation or confirmation of the acceptance of the PR. Once accepted the PR is merged into hotfix.

Now, clearly the bottlenecks are:

1. Junior maintainers time to confirm bugs and test PRs
2. Senior maintainers to provide insight and alternative solutions.

We currently only have 27 ‘Assessed’ Prs out of the 240+ which isn’t a lot and obviously time consuming for the juniors.

As spoken previously we have increased resource to oversee Github’s testing and label handling, but there is still a backlog of issues and PRs which needs additional resource to combat. Hence why we are here!

Pre-note before the proposed solution. There are specifics that need to be finalised to fully put this in place – review of labelling (introducing new more specific labels) which we can confirm later.

[size=5]Proposed Solution[/size]

Clearly the first hurdle is testing – testing if a bug exists and if a PR linked to an issue resolves it.

What we would like is to introduce a more detail bug reporting process that would allow the Community to assist with confirming that the issue is a bug or request more information from the original poster. Now I know a lot of members do this already (shout out to you all!) but there is no where that documents this process and keeps everyone on the same path. That is on our to do list to put this process in place.

A bug report would provide the Community a clear guide to how to test and progress a bug from non label to either Confirmed or Unconfirmed allowing SA to label these bugs with the ‘Low’, ‘Medium’, ‘High’ Priority states and then assign them out to available resource.

Below is the proposed Bug Reporting that any community member can participate (highly encouraged to do so )

[size=4]Bug Reporting[/size]

Community Steps for reviewing a Bug Report (aka an issue raised):

1. Is the Issue clear?

Has the Original Poster provided enough information to reproduce the bug? Is the bug easily reproduced?

2. Update Issue

Add a comment to the issue stating the outcome of the testing – of course thank the user for raising the issue :thumbs-up: Include the line Status: and we can quickly search for Issues that have that within the comment body to label accordingly.

Pending Input
This is where the OP hasn’t provided enough information to replicate it, please request for the missing information.

Unconfirmed
This is where the information provided was enough to reproduce the error BUT it works for your instance (or version). Use this Status also to state if you feel this is a feature rather than a bug, and provide a short explanation of why.

Confirmed
This is where you were able to replicate the issue and confirm that it is in fact a bug. You can state what level of priority this bug should be and any additional information to reproduce the bug would be extremely beneficial.

Once these Statuses are put in place, bugs can then be assigned out either to the Community – stating if the fix is a ‘Quick Fix’ as suggested on Github – or SalesAgility.

Note – In the future it would be ideal to have a bot that automatically updates labels by the status similar to Jekylls project, but we can search at the moment to tackle these requests.

(to be continued)

[samus-aran]

[size=5]Pull Request Reviewing
[/size]
Similar to the Bug testing, but understandably testing Pull Requests will take longer to review as it requires somewhat knowledge of the language, knowledge of git to pull down and test the branch (something we will document to assist).

Pick a Pull Request that has either no label or ‘Needs Review’ and work through the below Steps. Just a note here that even though you have have tested a PR the final say is with the senior maintainer and how to best implement the Fix and which release it will be scheduled into. However, if the more bugs are confirmed and are confirmed as resolved it allows users to immediately apply those fixes to their own instances if they do not wish for the next release.

1. Is the PR Complete?

Does it contain the linked issue - please see our ‘How to Contribute’ page on the wiki. Prs that are linked to an issue MUST contain a commit message linking it to said issue. Simply referencing the issue within the body of the PR is not enough (and a pain in our Release Notes generation).

Fixed #1234 -

Does it also contain all the steps to confirm the fix has been resolved and appropriate attributes i.e. does it break functionality of other areas etc.

2. Is the Base Branch Correct?

We currently provide two branches – hotfix and develop. With the introduction of the LTS we will be more strict with what is a feature introduction and a bug fix. Hotfixes are bug fixes and Features will be merged into develop branches.

3. Reproduce the problem.

If the same community member confirmed the issue was reproducible it would be beneficial that they were able to confirm the proposed PR has resolved the issue. But that isn’t essential. As long as you are able to reproduce the issue following the Steps within the linked issue then you can review the PR.

4. Review the Code

Now, this isn’t required as the senior maintainer will do this anyway regardless if neither community member does this. However we’ll include this step anyway.

++ Does the code only address the issue at hand I.e doesn’t contain more than one fix within the PR.
++ Does the code follow our Coding Standards?
++ Does the code break backwards compatibility including SuiteCRM versions and environments e.g MSSQL or PHP versions.

Some of the above will be eventually automatically tested (more thoroughly in the future).

5. Test the Code

Community members can pull down from other Contributor’s forks to test. A detailed step by step process will be documented.

6. Update Pull Request

As a community member you can create a ‘Review’ via the ‘Files changed’ tab on any Pull Request. You can either Comment, Approve or Request changes. For the sake of keeping it simple I would suggest the following two are used:

Request changes

If the Pull Request is not complete and/or the code does not resolve the issue. This also includes that the code breaks another area of the instance. Update with a comment explaining the issues and require changes before approving.

Approve Review

State that you have tested the code and confirm that it has resolved the issue and you are satisfied with all the checks made. A PR with two successful reviews by two different contributors will alert the senior maintainer to do the final review and decide whether it can be merged or needs further work.

Now, we will see how the above goes but there may be situations where Request changes are only suggestions and not required. If that being the case only raise them as comments rather than placing them in as part of the request changes requirements comment.

[size=4]Pull Requests Reviewed by Community[/size]

As stated above, each Pull Request must be approved by two contributors (not including the original poster of the Pull request) before the Pull Request is moved to final review stage.

Now, there will be cases that a Pull request will be fast tracked to the final review due to impact or priority and there will be cases that a Pull Request will go back and forth between the final review even after the Pull Request has been reviewed. In the latter case the Pull Request may have its Reviewed status revoked to ensure updated code will be tested again – depending on how drastic the code changes were.

[size=4]Enhancements/Features[/size]

Enhancements (later to be known as features) will have an additional step. These would be assessed by a senior maintainer to confirm that the feature is within the scope of the project. If so then the feature can be assigned to the community to test and follow the same process as the bug PR will adhere to.

(to be continued)

[samus-aran]

[size=4]Future Possibilities[/size]

Additional to the Community Reviews we have a few other ideas to make ours and the Community’s progress more transparent. These are only future ideas and nothing has been set in stone though some more than other. More than happy to hear your feedback on other ways to maintain a steady and practical way of processing bugs and Prs on our Github project.

++ Focus on the automated tests – currently we have Travis CI and basic test suite but this needs improved.
++ Introduce label assigning bot
++ Introduce git projects to schedule minor releases and assigned Contributors bugs/features – perhaps a ‘Help Wanted’ label.
++ Bug/PR party using Gitter - two members of SA (one senior and one junior) to assist with bug reporting, PR reviewing and general discussion.

[size=5]Actions[/size]

So we have the outline above to provide the Community more access to push bugs along, and allows SA to (hopefully) relieve the resource for purely testing Prs. However there are still ‘To Dos’ that we need to finalise before cracking ahead with the above.

++ Review and Finalise Labels to assist with new Community Reviewing process
++ ++ Document these labels clearly.
++ SA to provide more senior maintainers to do the final reviews
++ Publish Community Reviewing process and advertise it on Github/Forums/public domain.

[size=4]Proposed Labels[/size]

Type:

bug
duplicate
enhancement (only for PRs)
invalid
question
suggestion

Priority:

Low Priority
Medium Priority
High Priority

Issue Statuses:

Pending Input
Unconfirmed
Confirmed
Fixed Proposed
Resolved: Next Release

Issue Attributes:

Help Wanted
Quick Fix

PR Statuses:

Needs Review
Needs Work (Review has been rejected)
Community Assessed
Assessed (SA own assessment)
In Review
On Roadmap
Wrong Branch

So if you are still here after reading that long process then please share your thoughts, concerns and of course other suggestions to how benefit both the community and the project!

[pgr]

Thanks for all that (I did read it all!), it’s good to see things falling into place.

A few thoughts:

1. The most important and decisive bottleneck is the final merge by a senior SA person. If that works well, all goes well. If that is missing, no amount of labeling / process improvement is going to help. So that should be a crucial internal metric for you: merge (say) 10 PR’s a week, never less. Keep that up consistently. Slowly the project will get back on track and things will start to function better.

2. Maybe a set of labels to differentiate how big a PR is, how much or risk it carries. Maybe “Low effort”, “High effort”. All the PR’s without these labels are considered “Medium effort”, without the need to actually label them so. Some really low effort PR’s should be merged on the spot: you see it, it’s obvious, you merge it, skip steps in the process. Some very high effort PR’s need you to set time aside, get into the issues, have meetings, etc.

3. Some PR’s are major overhauls of some aspect of the app, touching dozens or hundreds of points in the code. These are hard/impossible to thoroughly test. However, they are the most important strategically to improve the quality of the code. Things like tests, logging, “requires”, some directory creation code that messes up permissions, etc.
   There could be a different strategy for them, like joining them together in a single Beta release that we would try to get more people to download and test, or assign each person a module so they would test that one thoroughly (e.g. I am in charge of Accounts, so I try every option related to Accounts). This way we could advance more boldly and still have some plan to keep risks under control. I think the reason these PR’s aren’t merged is simple, understandable, fear of the risk involved in messing with so many lines of code at once. But we need a plan for these, we need to get these merges going, otherwise we will stall the most needed, deep changes to the code.

[horus68]

I only read this today after my Github stats (I was on a leave!)

• I agree with almost everything detailed here.

• Request: Label “Language” when a PR also included language changes. It will help me to find the ones that will be necessary to be included in the Crowdin files!

• On the problems you face is the so many bugs already fixed in the 244 PR not merged, some are too old.
  Its difficult to test an old PR as we already imagine it will take even longer to be merged and collisions would happen when other parts of the code are changed in a more recent PR. Its really frustrating!
  That’s why the PR parties (or planned PR tests) are required so the PR that are not popular can be solved (merged or removed).

[abuelodelanada]

Hi Ashley!,

Now, clearly the bottlenecks are:

1. Junior maintainers time to confirm bugs and test PRs
2. Senior maintainers to provide insight and alternative solutions

What if we create a group made up of SalesAgility developers and Partners and Community developers so that these bottlenecks do not occur?

Best regards
Jose

[samus-aran]

[quote=“pgr” post=46405]Thanks for all that (I did read it all!), it’s good to see things falling into place.

A few thoughts:

1. The most important and decisive bottleneck is the final merge by a senior SA person. If that works well, all goes well. If that is missing, no amount of labeling / process improvement is going to help. So that should be a crucial internal metric for you: merge (say) 10 PR’s a week, never less. Keep that up consistently. Slowly the project will get back on track and things will start to function better.

2. Maybe a set of labels to differentiate how big a PR is, how much or risk it carries. Maybe “Low effort”, “High effort”. All the PR’s without these labels are considered “Medium effort”, without the need to actually label them so. Some really low effort PR’s should be merged on the spot: you see it, it’s obvious, you merge it, skip steps in the process. Some very high effort PR’s need you to set time aside, get into the issues, have meetings, etc.

3. Some PR’s are major overhauls of some aspect of the app, touching dozens or hundreds of points in the code. These are hard/impossible to thoroughly test. However, they are the most important strategically to improve the quality of the code. Things like tests, logging, “requires”, some directory creation code that messes up permissions, etc.
   There could be a different strategy for them, like joining them together in a single Beta release that we would try to get more people to download and test, or assign each person a module so they would test that one thoroughly (e.g. I am in charge of Accounts, so I try every option related to Accounts). This way we could advance more boldly and still have some plan to keep risks under control. I think the reason these PR’s aren’t merged is simple, understandable, fear of the risk involved in messing with so many lines of code at once. But we need a plan for these, we need to get these merges going, otherwise we will stall the most needed, deep changes to the code.[/quote]

4. Agreed, and we have already tried the merge X amount a week which failed due to other client paying commitments. Now we have got some more additional senior devs to review (the Product Team) that will be taking over in regards to reviewing (you know them as Daniel & Gyula) so thats the first steps getting back to that KPI target.

5. Ok, yeah I can see that. So say for languages and text changes can easily fall into that category.
   I’m interested in using the label:¯_(ツ)_/¯ ? Haha, perhaps not.

Though I think having the detailed different levels of PR difficulties will include more work to assess and categorise them than just to have a simple ‘Low effort’. More harder ones should be obvious and its up the tester/review to want to tackle them.

Another Status would be ‘Needs Decision’ so this would be more of a biggy and as you’ve said an overhaul of things.

3. That’s a good idea. Perhaps move most into a specific develop branch and out with the usual SuiteCRM release cycle include pre-release versions or even dare I say nightly builds… (ambitious how about bi-weekly builds) to allow the really big changes to be available to touch upon. Certainly something to think about!

[quote=“horus68” post=46621]I only read this today after my Github stats (I was on a leave!)

• I agree with almost everything detailed here.

• Request: Label “Language” when a PR also included language changes. It will help me to find the ones that will be necessary to be included in the Crowdin files!

• On the problems you face is the so many bugs already fixed in the 244 PR not merged, some are too old.
  Its difficult to test an old PR as we already imagine it will take even longer to be merged and collisions would happen when other parts of the code are changed in a more recent PR. Its really frustrating!
  That’s why the PR parties (or planned PR tests) are required so the PR that are not popular can be solved (merged or removed).[/quote]

Ok adding an Language label is suitable – may consider other very generic ‘types’ of topics also.
PR parties I’m actually keen to do 1. brings us into the community which we don’t do enough of 2. you are right, allows us to discuss the many concerns a PR may cause, things we haven’t thought of and even if need be allocate responsibility and actions. I will definitely try and set something up early Summer to test the waters.

[quote=“abuelodelanada” post=46622]Hi Ashley!,

Now, clearly the bottlenecks are:

1. Junior maintainers time to confirm bugs and test PRs
2. Senior maintainers to provide insight and alternative solutions

What if we create a group made up of SalesAgility developers and Partners and Community developers so that these bottlenecks do not occur?

Best regards
Jose
[/quote]

I am so glad you said that ;D We are in the midst of reaching out (once we get this Github process more finalised) to the Partners and seek assistance with reviewing and using their vast wealth of knowledge to bring these Prs in.

I believe it was mentioned in another thread/post regards to using the dedicated members of the community and inviting them into these larger sessions - I think that would be good for the Community having advocates. Would also be good to have a dedicated channel of communication that solely deals with github issues and PRs (regardless of PR parties or not).

Lots to think (and do) about!