# Bad PR Example

This example demonstrates common mistakes that annoy maintainers and lead to PR rejection or delays. Learn what NOT to do!

---

## Example: Adding OAuth2 Authentication (Done Wrong)

### PR Title ❌
```
Updated code
```

**Why it's bad:**
- ❌ Too vague
- ❌ Doesn't describe what changed
- ❌ No type prefix (feat/fix/docs)
- ❌ Wrong tense (past instead of imperative)
- ❌ Lowercase (should capitalize)

**Should be:**
```
feat(auth): add OAuth2 support for Google and GitHub providers
```

---

### PR Description ❌

```markdown
## Changes
Added OAuth

## Testing
Works for me
```

**Why it's bad:**
- ❌ No explanation of WHAT was added
- ❌ No explanation of WHY it's needed
- ❌ No explanation of HOW it works
- ❌ No testing instructions for reviewers
- ❌ No issue links
- ❌ No screenshots
- ❌ "Works for me" is not helpful

**Result:** Maintainer has to ask multiple questions before even starting review.

---

### Files Changed (Messy!) ❌

```
.env                             # ❌ SECRETS COMMITTED!
.vscode/settings.json            # ❌ Personal IDE config
SESSION.md                       # ❌ Personal development notes
planning/oauth-implementation.md # ❌ Planning documents
screenshots/debug-oauth-1.png    # ❌ Debug screenshots
screenshots/debug-oauth-2.png    # ❌ Debug screenshots
screenshots/test-local.png       # ❌ Personal testing screenshots
test-manual.js                   # ❌ Temporary test file
NOTES.md                         # ❌ Personal notes
TODO.md                          # ❌ Personal todo list
node_modules/                    # ❌ Dependencies (should be in .gitignore!)
package-lock.json                # ❌ Lock file changes (not needed)
src/routes/auth.js              # ✅ Actual feature code
src/routes/users.js             # ❌ Unrelated changes
src/routes/posts.js             # ❌ Unrelated changes
src/components/Header.js         # ❌ Unrelated changes
src/utils/formatting.js         # ❌ Unrelated refactoring
README.md                        # ✅ Documentation update
docs/authentication.md           # ✅ OAuth documentation
```

**Problems:**
- ❌ **28 files changed** (way too many!)
- ❌ **Secrets committed** (.env file with API keys)
- ❌ **Personal artifacts** (SESSION.md, NOTES.md, planning/)
- ❌ **Unrelated changes** (users.js, posts.js, Header.js)
- ❌ **Mixed concerns** (feature + refactoring + bug fixes)

**Should be:** 5-8 files, only those directly related to OAuth feature.

---

### Commit History (Terrible!) ❌

```
WIP
fixed stuff
asdf
more changes
fix
actually working now
Final commit
Actually final
OK this one is final
oops forgot something
```

**Why it's bad:**
- ❌ 10 commits for one feature (should squash)
- ❌ Meaningless commit messages
- ❌ "WIP" commits (not production-ready)
- ❌ No explanation of what changed
- ❌ No issue links
- ❌ Commit history shows trial-and-error (should clean up)

**Should be:** 1-2 clean commits with proper messages.

---

### SESSION.md Content (Should Never Be Committed!) ❌

```markdown
# OAuth Implementation Session

## 2025-11-04
Started working on OAuth. Not sure if I should use passport or just implement it myself. Going to try passport first.

## 2025-11-05
Passport is confusing. Spent 3 hours debugging. Finally got it working but the code is messy.

TODO:
- Clean up the callback logic
- Add tests (maybe)
- Figure out how to handle errors
- Ask @maintainer about rate limiting?

NOTES:
- Google OAuth app ID: 123456789-abcdefgh.apps.googleusercontent.com
- Redirect URI: http://localhost:3000/auth/google/callback
- Remember to add to staging: https://staging.myapp.com/auth/google/callback

BUGS FOUND:
- Header component has alignment issue on mobile
- Post creation form doesn't validate correctly
- User profile page crashes when avatar is null
```

**Why this should NEVER be in a PR:**
- ❌ Personal development journal
- ❌ Reveals your confusion/struggle
- ❌ Contains unfinished TODOs
- ❌ Mentions unrelated bugs
- ❌ Unprofessional appearance
- ❌ Pollutes the project

---

### NOTES.md Content (Should Never Be Committed!) ❌

```markdown
# Development Notes

## OAuth Research
Looked at how GitHub and GitLab do OAuth. Their implementations are pretty complex. Mine is simpler.

## Design Decisions
- Using passport because it's easier
- Not implementing token refresh yet (can do later)
- Rate limiting - should probably add this but skipping for now
- Testing - added some tests but not complete coverage

## Things I'm Not Sure About
- Is the error handling good enough?
- Should I use sessions or JWT?
- Do I need to validate the email from OAuth providers?

## Known Issues
- Doesn't work in Safari (CORS issue)
- Memory leak in callback handler (need to fix)
- Missing rate limiting (security risk?)
```

**Why this hurts your PR:**
- ❌ Shows incomplete work
- ❌ Admits to known issues not mentioned in PR
- ❌ Reveals security concerns not addressed
- ❌ Makes maintainer lose confidence
- ❌ Creates more work for maintainer

---

### Communication During Review ❌

**Initial Comment:**
```
Here's my OAuth implementation. Let me know what you think.
```

**Why it's bad:**
- ❌ No context
- ❌ No explanation
- ❌ No testing instructions
- ❌ Sounds careless

---

**Response to Feedback (Poor):**

**Maintainer:** "Could you add tests for the error cases?"

**Bad Response:**
```
Tests are boring. The code works fine without them.
```

**Why it's bad:**
- ❌ Dismissive
- ❌ Unprofessional
- ❌ Doesn't follow project standards
- ❌ Shows lack of respect

---

**Maintainer:** "This PR is too large. Could you split it into smaller PRs?"

**Bad Response:**
```
It's all related though. I don't want to spend time splitting it up.
```

**Why it's bad:**
- ❌ Refuses reasonable request
- ❌ Doesn't consider reviewer's time
- ❌ Makes review harder
- ❌ Likely to be closed

---

**No Response for 2 Weeks** ❌

**Maintainer:** "The tests are failing. Can you fix them?"

**Your response:** [Silence for 2 weeks]

**Why it's bad:**
- ❌ Wastes maintainer's time
- ❌ PR goes stale
- ❌ Likely to be closed
- ❌ Damages your reputation

---

### PR Metrics ❌

**Size:**
- Lines changed: 847
- Files changed: 28
- Commits: 10

**Problems:**
- ❌ Way too large (should be <200 lines)
- ❌ Too many files (includes unrelated changes)
- ❌ Messy commit history

**Timeline:**
- Submitted: Day 1
- Maintainer requests changes: Day 2
- No response: Days 3-16
- PR closed as stale: Day 17

---

## Specific Mistakes Breakdown

### 1. Committed Secrets ❌

**.env file contents:**
```
GOOGLE_CLIENT_ID=123456789-abcdefgh.apps.googleusercontent.com
GOOGLE_CLIENT_SECRET=GOCSPX-abc123def456  # ❌ SECRET!
GITHUB_CLIENT_ID=Iv1.1234567890abcdef
GITHUB_CLIENT_SECRET=1234567890abcdef1234567890abcdef12345678  # ❌ SECRET!
DATABASE_URL=postgresql://admin:SuperSecret123@localhost/myapp  # ❌ PASSWORD!
```

**Impact:**
- 🚨 Security breach!
- 🚨 Must rotate all secrets immediately
- 🚨 Potentially compromises production
- 🚨 Even if removed later, it's in git history

**What you should have done:**
- ✅ Only commit .env.example with placeholder values
- ✅ Add .env to .gitignore
- ✅ Never commit actual secrets

---

### 2. Including Unrelated Changes ❌

**src/routes/users.js:**
```javascript
// OAuth PR includes this "drive-by fix"
exports.getUser = async (req, res) => {
  const user = await User.findById(req.params.id);
  // Fixed bug where avatar URL was broken
  if (user.avatar) {
    user.avatar = user.avatar.replace('http://', 'https://');
  }
  res.json(user);
};
```

**Why it's bad:**
- ❌ Not related to OAuth feature
- ❌ Makes PR harder to review
- ❌ Mixed concerns
- ❌ If PR is reverted, this fix goes too

**What you should have done:**
- ✅ Create separate PR for bug fix
- ✅ Keep OAuth PR focused on OAuth only

---

### 3. Massive PR with No Breakdown ❌

**Includes all at once:**
- OAuth implementation (200 lines)
- Refactoring of existing auth (300 lines)
- Bug fixes in unrelated files (150 lines)
- UI updates (100 lines)
- Test updates (97 lines)

**Total: 847 lines in 28 files**

**Why it's bad:**
- ❌ Takes hours to review
- ❌ Hard to find bugs
- ❌ Difficult to discuss
- ❌ Can't merge incrementally

**What you should have done:**
- ✅ PR #1: Refactor existing auth (300 lines)
- ✅ PR #2: Add OAuth backend (200 lines)
- ✅ PR #3: Add OAuth UI (100 lines)
- ✅ PR #4: Fix related bugs (150 lines)

---

### 4. Poor Testing ❌

**test-manual.js (Committed by mistake!):**
```javascript
// Quick test script - DELETE BEFORE COMMIT!
const axios = require('axios');

async function testOAuth() {
  // This only works on my machine
  const response = await axios.get('http://localhost:3000/auth/google');
  console.log('Works!');
}

testOAuth();
```

**Why it's bad:**
- ❌ Not a proper test
- ❌ Hardcoded localhost
- ❌ No assertions
- ❌ Comment says "DELETE BEFORE COMMIT"
- ❌ Clutters project

**What you should have done:**
- ✅ Delete this file
- ✅ Write proper tests in tests/auth/oauth.test.js
- ✅ Use project's testing framework
- ✅ Include assertions and edge cases

---

### 5. Missing Documentation ❌

**README.md changes:**
```markdown
## Authentication

We now have OAuth.
```

**Why it's bad:**
- ❌ No setup instructions
- ❌ No explanation of how it works
- ❌ No examples
- ❌ Unhelpful to users

**Should be:**
```markdown
## Authentication

### OAuth 2.0 Social Login

Users can sign in with Google or GitHub accounts.

#### Setup

1. Create OAuth apps:
   - Google: https://console.cloud.google.com/apis/credentials
   - GitHub: https://github.com/settings/developers

2. Add credentials to `.env`:
   ```
   GOOGLE_CLIENT_ID=your_client_id
   GOOGLE_CLIENT_SECRET=your_client_secret
   GITHUB_CLIENT_ID=your_client_id
   GITHUB_CLIENT_SECRET=your_client_secret
   ```

3. Restart server

#### Usage

Users will see "Sign in with Google" and "Sign in with GitHub" buttons on the login page.

For detailed implementation, see [docs/authentication.md](docs/authentication.md).
```

---

### 6. Ignoring CI Failures ❌

**CI Status:**
```
❌ Tests: 5 failing
❌ Lint: 23 errors
❌ Build: Failed
```

**Your response:** Submit PR anyway, hope maintainer doesn't notice

**Why it's bad:**
- ❌ Shows you didn't test
- ❌ Wastes CI resources
- ❌ Can't merge with failing CI
- ❌ Unprofessional

**What you should have done:**
- ✅ Fix all CI issues before submitting
- ✅ Run tests locally first: `npm test`
- ✅ Run lint locally: `npm run lint`
- ✅ Only submit when all checks pass

---

## The Result

**Maintainer's Response:**
```
Thanks for the PR, but there are several issues:

1. You've committed secrets in the .env file - this is a security issue
2. The PR includes unrelated changes (users.js, posts.js, Header.js)
3. Personal development files (SESSION.md, NOTES.md, planning/) shouldn't be here
4. The PR is too large - 847 lines across 28 files
5. Tests are failing
6. Missing proper documentation
7. test-manual.js is committed

Please:
- Remove all secrets and rotate them
- Create separate PRs for unrelated changes
- Remove personal development artifacts
- Fix the failing tests
- Add proper documentation
- Keep the PR focused on OAuth only

I'm closing this for now. Please feel free to submit a new PR addressing these issues.
```

**Status:** ❌ PR Closed

---

## Key Lessons

### What Went Wrong

1. **Security**: Committed secrets (.env file)
2. **Scope**: Too large, too many unrelated changes
3. **Artifacts**: Personal files committed (SESSION.md, NOTES.md)
4. **Testing**: Poor testing, CI failures
5. **Documentation**: Inadequate documentation
6. **Communication**: Poor responses to feedback
7. **Quality**: Messy commits, no code review
8. **Professionalism**: Dismissive attitude

### How to Fix It

1. **Security**
   - Never commit secrets
   - Use .env.example with placeholders
   - Run pre-PR check script

2. **Scope**
   - Keep PRs small (<200 lines)
   - One feature per PR
   - No unrelated changes

3. **Artifacts**
   - Remove SESSION.md, NOTES.md, TODO.md
   - Remove planning/ directory
   - Remove debug screenshots
   - Use clean-branch script

4. **Testing**
   - Write proper tests
   - Fix CI before submitting
   - Test locally first

5. **Documentation**
   - Update README
   - Add setup instructions
   - Include examples

6. **Communication**
   - Be responsive
   - Be respectful
   - Accept feedback gracefully

7. **Quality**
   - Clean commit history
   - Proper commit messages
   - Review your own code

8. **Professionalism**
   - Respect maintainer's time
   - Follow project conventions
   - Be patient and courteous

---

## Comparison: Bad vs Good

| Aspect | Bad PR ❌ | Good PR ✅ |
|--------|-----------|-----------|
| **Title** | "Updated code" | "feat(auth): add OAuth2 support" |
| **Size** | 847 lines, 28 files | 180 lines, 9 files |
| **Commits** | 10 messy commits | 1 clean commit |
| **Files** | Includes SESSION.md, .env | Only relevant files |
| **Testing** | test-manual.js, CI failing | Proper tests, CI passing |
| **Docs** | "We now have OAuth" | Complete setup guide |
| **Secrets** | Committed .env | Only .env.example |
| **Scope** | OAuth + bugs + refactor | OAuth only |
| **Communication** | Dismissive | Professional |
| **Result** | Closed | Merged in 3 days |

---

**Remember:** Every mistake in this example is based on real PRs that have been rejected. Learn from these errors and follow the good PR example instead!