Test.md

Android PRs Workflow

Weclome! This is an RFC on how we can ease the bottle neck of the review process on the Android Team

Problem Statement

The main bottleneck we have in developing Android at Stash is the pull review process, as is the case in most shops. For that reason I'm putting together guidelines and example workflows for us to follow in order to alleviate that pain point.

We have tried a few things in the past to help with this process which all had lessons we learned. The please-review command we found was great distributing the reviews across the team to prevent domain experts as well as getting two sweeps on the pr. These were all great but we still are experiencing the bottleneck in getting code merged. We also talked about limited the size of the number of requests outstanding and/or limited the number of open pull request per person. These are another great idea which has shown value in keeping the review number under control but again didn't fix the bottleneck.

I believe that the reason for this bottleneck at the review step is due to the complex nature of the majority of pull requests. Our current workflow for feature development:

1. Create a feature branch eg feature/prpl-420
2. Build the complete feature in the branch
3. Submit for review
4. Address comments/feedback
5. Merge to develop

These complex and large pull requests take a ton of time to review and are often put on the back burner for the reviewer until they can allocate the proper time which may be hours or days.

We have taken all the lessons learned in the past and created the next evolution of the process which is what this document is detailing.

I would assert that creating small and more concise PRs would help fix our bottleneck because:

1. The prs would be small which would increase the review speed and merging throughput.
2. It's easier to context switch quickly to a PR that doesn't require you to thoroughly dig in the code to see what the author is doing. In other words our PRs should reflect some of our coding methodologies of writing clean, clear, and concise code. The same should be applied to prs.

Proposal

Solution for this bottleneck

Breaking PRs up into smaller distinct parts, when possible.

• Breaking up the pr will require planning which means better User Stories
  • This means creating more subtasks in Jira which will give product much more transparency and clarity on what you are working
  • Stories will be estimated more accurately
  • This will allow for more rapid and meaning reviews which will increase to higher turnover time.

Breaking tickets or features into distinct parts. Devs can make subtasks in jira which will help create more transparency with product

Example Flow

Feature to implement Bank link flow ?

Product / Planning workflow:

Break the task into subtasks

1. Each PR will have a Task and Subtask ticket associated with it. The task being the implementation of the overall feature and the subtask being the broken down tasks in the pr
2. This can be done in a PBR with a PM or at a devs lesuire
3. You can also use the /jira-create command to quickly make one (I'll add subtask option)

Engineering feature workflow after product planning¹:

1. Convert to kotlin
2. Create skeleton Flow
   • Should only need a feature flag if code is reachable or may affect production if merged
3. Move / Copy / Delete classes
4. Create services / networking stack if needed
5. Create Screens
   • There should be a PR for each screen.
   • (Joel help with the adapter or whatever for new models)
6. FINAL remove config flag or hookup gateway so users/dev can access the new code/flow

¹ Each step is a distinct PR

Edge cases

These are guidelines not rules. As such they are flexible. There are certain cases in the real world that we will encounter that have no great solution.

Examples:

1. Moving packages, deleting packages, changing file names, etc.
   • The changes should be in a single PR
   • If you need logic in order for it to compile it is the authors responsibility to surface where those changes are in the pr. It can be done in the PR description or by commenting inline where you need the additional care taken to review.

Benefits

• Ppl will take more time to think about implementing their features.
• Review time will be drastically decrease because of small easy to digest PRs.
• Review quality will increase because prs will be clear and concise so the reviewer won't struggle with understanding context of the change as much.
• Once familiar with process dev speed should increase because they will be blocked less.
• Generally large prs have tests that get lost. With smaller scoped prs the tests become much more clear what parts they are testing and what parts may have been missed.
• Less time reviewing, more time we can spend doing the fun part of our job
• Fewer bugs
• Better throughput overall

Implementation

• All devs must be hold each other accountable for enforcing sub tasks / pr size / best practices.
• PRs for large features will now require a parent ticket (epic/story/etc) and a subtask (the actual content of the PR). These will be added to the PR template and will be required for large features.
• Git pre commit hooks that warn devs from committing more than x (More than 20? See Measuring Success) amount of files.
  • The message would have links to our wiki about PRs, best practices, and other useful resources
  • Also quote the golden review - "Put yourself in the reviewers shoes. If you put out a PR would you want / have time to review it? If the answer is no then you need to rethink the change."
  • The final message "Are you sure u know what ur doing [y/n]?". It would never block anyone from commiting code.
• Using Stash config or Apptimize to flag features off until they are production ready

Measuring Success

I will be doing metrics on if this method is impactful. I currently have collected data on the last 100 PR (Github rate limit issues) and found this:

< 22 files change 	
23.20011029(hrs)	

> 22 files changes
33.84543561 (hrs)

So there is a 70% increase in review time in prs over 22 files vs the avg of review time of 22files changes or below

Questions

Q: How do I know when I should make a sub ticket? A: If you touching differnt domains of the app eg: UI, Networking, etc then the feature should be broken up. If you creating multiple screens then it should be broken up.

Q: What is the MVP for subtask? A: If you can make a subtask that someone can do in parallel to your work then it's a valid ticket.