Discussion: Internal - Pull Requests management

[machour]

This is an attempt to lay out some ground rules in order to improve our handling of PRs.

Hopefully, this will avoid finding ourselves in the current situation introduced by styled-components PRs.

I believe this should be part of some guidelines documentation once discussed & approved.

Rules for merging PRs

Base rules

• Every person who merges a PR should have tested it by itself on at least one device.
• Every merged PR should have a related issue (merger can create one if needed).
• Never merge your own PRs.

Exceptions

• Doc: a PR that only changes documentation files (contributors, readme, ..)
• Typo: a PR that fix a typo in translations or in some variable naming
• QuickFix: a PR that fixes an obvious mistake in js logic causing a know issue (bad if condition, ..)
• Crash: a one-linish PR that fix a crash in master.

Additionally, for those cases, if the person submitting the PR is a maintainer, he can commit directly to master (or merge his own PR without approval).

Tests

At some point, we will need to start requiring tests for every significant change.

We still need to have more diversified tests samples available in order to ease the task on the pull requester.

Enforcements

• UI: If the PR introduces a new UI, or a change in the UI (styled-components, ..), the PR should be tested on both iOS & Android by the person who merges it.
• UI: The PR needs to have screenshots for both Android & iOS. (either the pull-requester or merger should provide them in the discussion)
• i18n: The PR have to be approved by at least one native speaker before merge can happen

[machour]

@housseindjirdeh @andrewda @alejandronanez @chinesedfan @alejandronanez @lex111 @jouderianjr Please chime in with your ideas 🙏

[chinesedfan]

Great suggestions! I want to mention reviewers count and the duration.

• PRs should be merged with at least 2 approved reviewers.
  • If submitted by a maintainer, the approval from the author is not counted.
• PRs should be opened for at least 1 day, except for those urgent fixes.

[housseindjirdeh]

I couldn't agree more with all this @machour. @chinesedfan do you think we should have at least 2 and not 1? Have no problem to use 2 if we all think it makes sense to have a more thorough look.

@machour I think this and #626 should be in our docs somehow, will you be okay with me including it into our contributing docs? Please let me know if that's okay

[andrewda]

I can work on a GitHub app/bot to help enforce some of these when I get some free time.

[chinesedfan]

@housseindjirdeh Yes, I prefer 2 different approvals. Normally, a PR merged in 1 week is not a big problem. Of course, I am glad to listen other ones' opinions.

[machour]

@housseindjirdeh Feel free to use those two tickets as a base for the docs.
I also feel we need to document the following things too:

• How to keep the repo fork in sync with master (I've seen some users struggling)
• The proper git workflow to get pre-commit hooks playing nice (git add file1 file2, then git commit, or else hooks like prettier won't work properly)
• Give some examples of commits messages (users are really confused with this hook and simply discard it using --no-verify)

Could you take care of creating a docs/ folder when integrating this guide and #626 ?

[machour]

We also need some way of distinguishing these two:

• validated: "wow, this PR looks cool"
• validated: "I have tested this PR, and having seen it in action, I validate it"

I feel like sometimes validation is used as a 👍 , and this should not be counted in the two validations @chinesedfan mentionned

[housseindjirdeh]

Thanks a million @andrewda <3

I'm loving all of this, I'll take care of creating a new docs directory with this guide and points entirely in the next few days 🚀

[jouderianjr]

I totally agreed with these rules :D +1 for the test before merge 💃

Also, I think like @chinesedfan, I do prefer 2 approvals before merge, would better have 2 persons reviewing something because we casually have different points of view and one person could find something that the other one doesn't 😄