# Pull Request Guidelines

This guide covers the complete pull request (PR) process for contributing code to Changemaker Lite V2, from creation to merge.

## Before Submitting a PR

### 1. Create or Find an Issue

**For features**:
- Search [existing issues](https://github.com/changemaker-lite/v2/issues) first
- If none exists, [create a feature request](https://github.com/changemaker-lite/v2/issues/new?template=feature_request.md)
- Wait for maintainer approval before implementing
- Discuss implementation approach in the issue

**For bugs**:
- Search [existing bug reports](https://github.com/changemaker-lite/v2/issues?q=is%3Aissue+label%3Abug) first
- If none exists, [create a bug report](https://github.com/changemaker-lite/v2/issues/new?template=bug_report.md)
- Include reproduction steps and expected vs actual behavior
- Verify bug still exists on latest `v2` branch

!!! tip "Avoid Wasted Effort"
    Always create an issue and get approval before spending time on a large feature. Maintainers may have alternative approaches or priorities.

### 2. Test Your Changes

Run all checks locally before submitting:

```bash
# Type checking
cd api && npx tsc --noEmit
cd admin && npx tsc --noEmit

# Linting
cd api && npm run lint
cd admin && npm run lint

# Unit tests
cd api && npm test
cd admin && npm test

# Integration tests (if applicable)
cd api && npm run test:integration

# Build
cd api && npm run build
cd admin && npm run build
```

**All checks must pass** before submitting PR.

### 3. Update Documentation

If your changes affect:

- **API endpoints**: Update [API reference](../api-reference/index.md)
- **User features**: Update [user guides](../user-guides/admin-guide.md)
- **Environment variables**: Update [`.env.example`](https://github.com/changemaker-lite/v2/blob/v2/.env.example)
- **Database schema**: Update [database docs](../database/schema.md)
- **Configuration**: Update [deployment guides](../deployment/production.md)

!!! warning "Documentation is Required"
    PRs with new features will not be merged without corresponding documentation updates.

## PR Title Format

Use **Conventional Commits** format:

```
type(scope): short description
```

### Types

| Type | When to Use |
|------|-------------|
| `feat` | New feature for users |
| `fix` | Bug fix |
| `docs` | Documentation changes |
| `style` | Code formatting (no behavior change) |
| `refactor` | Code restructuring (no behavior change) |
| `perf` | Performance improvements |
| `test` | Test additions or fixes |
| `chore` | Build process, tooling, dependencies |
| `ci` | CI/CD configuration |
| `revert` | Reverting a previous commit |

### Scopes

Common scopes by area:

**Backend**:
- `api` - General API changes
- `auth` - Authentication/authorization
- `campaigns` - Campaign module
- `locations` - Location module
- `shifts` - Shift module
- `canvass` - Canvassing module
- `email` - Email sending
- `database` - Database schema/migrations

**Frontend**:
- `admin` - Admin pages
- `public` - Public pages
- `volunteer` - Volunteer portal
- `components` - React components
- `store` - Zustand stores
- `ui` - UI/UX changes

**Infrastructure**:
- `docker` - Docker/Docker Compose
- `nginx` - Nginx configuration
- `monitoring` - Prometheus/Grafana
- `deployment` - Deployment scripts/config

### Examples

**Good titles**:
- ✅ `feat(campaigns): add campaign export to CSV`
- ✅ `fix(geocoding): handle null responses from Nominatim`
- ✅ `docs(api): document campaign endpoints`
- ✅ `refactor(auth): extract JWT middleware to separate file`
- ✅ `perf(locations): add database index on postalCode`

**Bad titles**:
- ❌ `Update campaigns.tsx` (too vague)
- ❌ `Bug fix` (no scope or description)
- ❌ `WIP: New feature` (don't submit WIP PRs)
- ❌ `Fixed everything` (not descriptive)

## PR Description Template

Use this template for your PR description:

```markdown
## What

[Clear description of what this PR does]

## Why

[Why this change is needed, link to issue]

## How

[Brief explanation of implementation approach]

## Testing

[How to test these changes]

## Screenshots

[For UI changes, include before/after screenshots]

## Checklist

- [ ] Tests added/updated
- [ ] Documentation updated
- [ ] No console errors
- [ ] All CI checks pass
- [ ] Follows code style guidelines

Fixes #[issue-number]
```

### Example PR Description

```markdown
## What

Adds a CSV export button to the campaigns page that allows admins to download all campaigns with their metadata.

## Why

Users need to export campaign data for reporting and analysis in external tools like Excel.

Fixes #456

## How

- Added export button to CampaignsPage header
- Created `/api/influence/campaigns/export` endpoint
- Implemented CSV generation using `csv-stringify` library
- Added SUPER_ADMIN/INFLUENCE_ADMIN role check

## Testing

1. Login as admin user
2. Navigate to Campaigns page (/app/influence/campaigns)
3. Click "Export CSV" button in page header
4. Verify CSV file downloads with correct data
5. Open CSV in Excel/Google Sheets to verify formatting

## Screenshots

![Export button in campaigns page](https://user-images.githubusercontent.com/export-button.png)

## Checklist

- [x] Tests added (export endpoint integration test)
- [x] Documentation updated (API reference, admin guide)
- [x] No console errors
- [x] All CI checks pass
- [x] Follows code style guidelines

Fixes #456
```

## Creating the PR

### 1. Push Your Branch

```bash
# Ensure your branch is up to date
git fetch upstream
git rebase upstream/v2  # or: git merge upstream/v2

# Push to your fork
git push origin feature/your-feature-name

# If you rebased, force push (with care!)
git push --force-with-lease origin feature/your-feature-name
```

### 2. Open PR on GitHub

1. Go to your fork on GitHub
2. Click **"Pull requests"** tab
3. Click **"New pull request"**
4. **Base repository**: `changemaker-lite/v2` **base**: `v2`
5. **Head repository**: `YOUR-USERNAME/changemaker-lite` **compare**: `feature/your-feature-name`
6. Click **"Create pull request"**
7. Fill out the PR template (see above)
8. Click **"Create pull request"**

### 3. Request Reviewers

- PRs are automatically assigned to maintainers
- You can request specific reviewers if you know who to ask
- For urgent PRs, mention `@changemaker-lite/maintainers`

## Code Review Process

### Automated Checks

After submitting, CI/CD runs these checks:

1. **Lint**: ESLint rules
2. **Type Check**: TypeScript compilation
3. **Tests**: Unit + integration tests
4. **Build**: Production build
5. **Security**: Dependency vulnerability scan

**Status badges** appear on your PR:

- ✅ **Green checkmark**: All checks passed
- ❌ **Red X**: Some checks failed
- 🟡 **Yellow dot**: Checks in progress

**Fix failing checks** before requesting review.

### Maintainer Review

A maintainer will review your code and provide feedback:

**Review categories**:

1. **Code quality**:
   - Follows code style guidelines
   - No unnecessary complexity
   - Proper error handling
   - No security vulnerabilities

2. **Functionality**:
   - Solves the problem correctly
   - Edge cases handled
   - No regressions

3. **Tests**:
   - Adequate test coverage (>80%)
   - Tests are meaningful
   - Tests pass consistently

4. **Documentation**:
   - Code comments for complex logic
   - API documentation updated
   - User guide updated (if needed)

### Review Outcomes

**Approved** ✅:
- Maintainer approves PR
- Ready to merge (after squash)

**Request Changes** 🔄:
- Maintainer requests modifications
- Address feedback and push new commits
- Re-request review after changes

**Comment** 💬:
- Feedback without blocking merge
- Optional to address

## Addressing Feedback

### 1. Read Feedback Carefully

- Understand the requested change
- Ask clarifying questions if unclear
- Don't take criticism personally (it's about code, not you)

### 2. Make Changes

```bash
# Make requested changes
# Edit files...

# Commit changes
git add .
git commit -m "refactor: address review feedback

- Extracted duplicate logic into helper function
- Added error handling for edge case
- Updated tests to cover new scenario"

# Push to same branch
git push origin feature/your-feature-name
```

**Commits are added to existing PR automatically.**

### 3. Respond to Comments

- **Acknowledge feedback**: "Good catch, fixed in abc1234"
- **Explain changes**: "Refactored this to use a switch statement instead"
- **Ask questions**: "I'm not sure how to handle X, suggestions?"
- **Mark resolved**: Click "Resolve conversation" after addressing

### 4. Re-Request Review

After addressing all feedback:

1. Click **"Reviewers"** section
2. Click circular arrow next to reviewer's name
3. Or comment `@reviewer Ready for re-review`

## Common Review Feedback

### Code Quality Issues

**Issue**: Large function with too many responsibilities

**Feedback**:
> This function is doing too much. Can you extract the geocoding logic into a separate function?

**Fix**:
```typescript
// Before
async function createLocation(data) {
  // 50 lines of validation, geocoding, database insert...
}

// After
async function createLocation(data) {
  const validated = validateLocationData(data);
  const geocoded = await geocodeAddress(validated.address);
  return insertLocation({ ...validated, ...geocoded });
}
```

**Issue**: Magic numbers or strings

**Feedback**:
> What does `30` represent here? Use a named constant.

**Fix**:
```typescript
// Before
if (visits.length >= 30) { }

// After
const VISIT_RATE_LIMIT = 30;
if (visits.length >= VISIT_RATE_LIMIT) { }
```

**Issue**: Missing error handling

**Feedback**:
> What happens if the API call fails? Add error handling.

**Fix**:
```typescript
// Before
const reps = await fetch(url).then(r => r.json());

// After
try {
  const response = await fetch(url);
  if (!response.ok) {
    throw new Error(`API returned ${response.status}`);
  }
  const reps = await response.json();
} catch (error) {
  logger.error('Failed to fetch representatives', error);
  throw new Error('Unable to lookup representatives');
}
```

### Test Coverage Issues

**Issue**: Missing test for edge case

**Feedback**:
> Add a test for when postal code is invalid.

**Fix**:
```typescript
it('should return 400 for invalid postal code', async () => {
  const response = await request(app)
    .post('/api/influence/representatives/lookup')
    .send({ postalCode: 'INVALID' });

  expect(response.status).toBe(400);
  expect(response.body.success).toBe(false);
});
```

### Documentation Issues

**Issue**: Missing API documentation

**Feedback**:
> Add this endpoint to the API reference docs.

**Fix**: Update `docs/v2/api-reference/campaigns.md` with new endpoint.

### Performance Issues

**Issue**: N+1 query problem

**Feedback**:
> This causes N+1 queries. Use Prisma `include` to join.

**Fix**:
```typescript
// Before (N+1)
const campaigns = await prisma.campaign.findMany();
for (const campaign of campaigns) {
  campaign.createdBy = await prisma.user.findUnique({ where: { id: campaign.createdByUserId } });
}

// After (single query)
const campaigns = await prisma.campaign.findMany({
  include: { createdBy: true }
});
```

## Merge Process

### Squash and Merge

Changemaker Lite uses **squash and merge** for all PRs:

1. Maintainer clicks **"Squash and merge"**
2. All commits in PR are squashed into one commit
3. Commit message = PR title + description summary
4. Merged to `v2` branch

**Why squash?**
- Clean linear history
- Easier to revert if needed
- No messy "WIP" or "fix typo" commits

### After Merge

Once your PR is merged:

1. **Celebrate!** 🎉 You've contributed to Changemaker Lite
2. **Update your fork**:
   ```bash
   git checkout v2
   git pull upstream v2
   git push origin v2
   ```
3. **Delete feature branch** (optional):
   ```bash
   git branch -d feature/your-feature-name
   git push origin --delete feature/your-feature-name
   ```
4. **Update issue**: GitHub auto-closes issue with `Fixes #N`
5. **Check release notes**: Your contribution will be mentioned in next release

## PR Checklist

Use this before submitting:

### Pre-Submission

- [ ] **Issue created** and approved by maintainer
- [ ] **Branch created** from latest `v2`
- [ ] **Changes implemented** following code style
- [ ] **Self-review** - read your own code critically
- [ ] **Manual testing** - verify changes work as expected

### Code Quality

- [ ] **TypeScript**: No type errors (`npx tsc --noEmit`)
- [ ] **Linting**: No lint errors (`npm run lint`)
- [ ] **Formatting**: Code formatted (`npm run format`)
- [ ] **No console logs**: Remove debug statements
- [ ] **No commented code**: Remove old code
- [ ] **Error handling**: All errors caught and logged

### Tests

- [ ] **Unit tests**: Added/updated tests
- [ ] **Tests pass**: `npm test` succeeds
- [ ] **Coverage**: Maintained or improved (>80%)
- [ ] **Integration tests**: Added if needed
- [ ] **Edge cases**: Tested invalid inputs

### Documentation

- [ ] **Code comments**: Complex logic documented
- [ ] **API docs**: New endpoints documented
- [ ] **User docs**: User guide updated (if user-facing)
- [ ] **README**: Updated if needed
- [ ] **.env.example**: New env vars added

### UI (if applicable)

- [ ] **Responsive**: Works on mobile/tablet/desktop
- [ ] **Accessibility**: Keyboard navigation works
- [ ] **Browser testing**: Works in Chrome, Firefox, Safari
- [ ] **Loading states**: Spinners for async operations
- [ ] **Error states**: Error messages shown to user
- [ ] **Screenshots**: Included in PR description

### Final Checks

- [ ] **CI passing**: All automated checks green
- [ ] **PR template**: Description complete
- [ ] **Commit messages**: Follow conventional commits
- [ ] **No merge conflicts**: Branch rebased/merged with `v2`
- [ ] **Reviewers requested**: Maintainers notified

## Troubleshooting PRs

### CI Checks Failing

**Lint failures**:
```bash
cd api && npm run lint:fix
cd admin && npm run lint:fix
git add . && git commit -m "chore: fix lint errors" && git push
```

**Type errors**:
```bash
cd api && npx tsc --noEmit  # Shows errors
# Fix type errors in code
git add . && git commit -m "fix: resolve type errors" && git push
```

**Test failures**:
```bash
cd api && npm test  # Run locally to see errors
# Fix failing tests
git add . && git commit -m "test: fix failing tests" && git push
```

### Merge Conflicts

**Resolving conflicts**:
```bash
# Fetch latest upstream
git fetch upstream

# Rebase onto v2
git rebase upstream/v2

# If conflicts, resolve them
# Edit conflicted files, then:
git add .
git rebase --continue

# Force push (since history changed)
git push --force-with-lease origin feature/your-feature-name
```

### PR Not Getting Reviewed

**If no review after 5 business days**:

1. **Check CI**: Ensure all checks pass
2. **Ping maintainer**: Comment "@changemaker-lite/maintainers Friendly ping for review"
3. **Join Discord**: Ask in #contributors channel
4. **Email**: dev@cmlite.org for urgent PRs

**Reasons for delays**:
- Maintainers busy with other priorities
- PR too large (break into smaller PRs)
- Missing context (add more details to description)
- Waiting on related PRs to merge first

## Related Documentation

- [Contributing Guide](index.md) - Overview
- [Development Setup](development-setup.md) - Environment setup
- [Code of Conduct](code-of-conduct.md) - Community standards
- [Roadmap](roadmap.md) - Future plans

## Questions?

- **Discord**: [#contributors channel](https://discord.gg/changemaker-lite)
- **Discussions**: [Ask in Q&A](https://github.com/changemaker-lite/v2/discussions)
- **Email**: dev@cmlite.org

Thank you for contributing! Every PR helps make Changemaker Lite better. 🚀