Code Quality Improvements and Bug Fixes

# Code Quality Improvements and Bug Fixes

This issue tracks code quality improvements, architectural issues, and bugs identified through comprehensive code analysis.

**Note:** These issues were identified by a code-specific AI and need manual verification and clarification.

---

## 🔴 CRITICAL ISSUES

### 1. Dual Storage Architecture Causing Data Loss

**Severity**: CRITICAL - Data will be lost
**Files**: Throughout codebase

The plugin has **two separate, unsynchronized data storage systems**:

- **System A**: Custom submission fields via schema extension (`CodecheckPlugin.php:176-199`)
- Data saved via `Submission::edit` hook
- Used by submission forms

- **System B**: `codecheck_metadata` table with DAO (`CodecheckSubmissionDAO.php`)
- Table created by migration
- Used by `ArticleDetails.php` to display certificates
- **Never populated** - the DAO insert/update methods are never called

**Impact**: Data entered during submission will NOT appear on published article pages.

**Recommended Fix**: Choose one approach:
- **Option 1** (Recommended): Remove the DAO/table entirely, use only custom submission fields
- **Option 2**: Add hook to sync submission data to table (duplicates OJS functionality)

```php
// Current: ArticleDetails tries to read from empty table
$dao = new CodecheckSubmissionDAO();
$codecheckData = $dao->getBySubmissionId($article->getId()); // Always returns null!

// Should use:
$codecheckData = $article->getData('codecheckOptIn');
```

---

### 2. Hard-coded Genre IDs Break File Validation

**Files**: `CodecheckPlugin.php:455-459`

```php
if ($genre && in_array($genre->getId(), [999, 998, 997])) {
```

These hard-coded IDs won't match dynamically created genres. File validation will always fail.

**Fix**: Query genres by designation or name:
```php
$genreDao = \PKP\db\DAORegistry::getDAO('GenreDAO');
$genre = $genreDao->getByDesignation('codecheck_yml', $context->getId());
```

---

### 3. SQL Injection Risk with Raw Query

**Files**: `CodecheckPlugin.php:145`

```php
$checkSql = "SELECT COUNT(*) as count FROM genre_settings WHERE setting_value = 'codecheck.yml'";
$result = $genreDao->retrieve($checkSql);
```

While this specific query is safe (no user input), using raw SQL is risky and against OJS best practices.

**Fix**: Use query builder or check programmatically:
```php
$genres = $genreDao->getByContextId($context->getId());
foreach ($genres as $genre) {
if ($genre->getLocalizedName() === 'codecheck.yml') {
return; // Already exists
}
}
```

---

## 🟠 HIGH PRIORITY ISSUES

### 4. Missing Exception Import

**Files**: `CodecheckSchemaMigration.php:54`

```php
} catch (Exception $e) { // Exception class not imported
```

**Fix**: Add `use Exception;` or use `\Exception`

---

### 5. Extensive Dead Code

**Unused classes/methods** (never called):
- `CodecheckSubmissionHandler` - entire class instantiated nowhere
- `validateCodecheckFiles()` - method defined but hook never registered
- `checkRequiredCodecheckFiles()` - only called by unused validateCodecheckFiles
- `addToReviewForm()` - method defined, hook never registered
- `debugSubmissionData()` - method defined, hook never registered

**Fix**: Either implement these features or remove the code.

---

### 6. Genre Creation Race Condition

**Files**: `CodecheckPlugin.php:127-172`

```php
static $genresCreated = false; // Only prevents duplicates in same request
```

Multiple concurrent requests can create duplicate genres. The SQL check only verifies one genre but creates three.

**Fix**:
1. Check all three genres before creating any
2. Use database unique constraints on `(context_id, designation)`
3. Handle duplicate insertion gracefully with try-catch

---

### 7. Production Logging Overload

**Files**: Throughout `CodecheckPlugin.php` (20+ instances)

```php
error_log("CODECHECK: Added all fields to titleAbstract form");
```

Excessive logging in production reduces performance and clutters logs.

**Fix**: Use conditional logging:
```php
if ($this->plugin->getSetting($context->getId(), 'debugMode')) {
error_log("CODECHECK: ...");
}
```

---

### 8. Inconsistent Data Format for Manifest Files

**Files**: `CodecheckPlugin.php:353-378` vs `js/submission-fields.js:66-82`

- JavaScript creates: `"filename - comment\nfilename2 - comment2"`
- PHP sometimes expects: JSON array of `{filename, comment}` objects
- `formatManifestFiles()` tries to handle both, but saving is inconsistent

**Fix**: Standardize on one format (recommend JSON for structured data)

---

## 🟡 MEDIUM PRIORITY ISSUES

### 9. God Object Antipattern

**Files**: `CodecheckPlugin.php` (568 lines)

Single file handles plugin registration, form building (3 forms), hook callbacks (7 hooks), genre management, data persistence, display rendering, and file validation.

**Fix**: Extract responsibilities into separate classes:
```
classes/
├── Forms/
│ ├── SubmissionStartFormHandler.php
│ ├── SubmissionDetailsFormHandler.php
│ └── ReviewDisplayHandler.php
├── FileManagement/
│ ├── GenreManager.php
│ └── FileValidator.php
└── Data/
└── CodecheckDataHandler.php
```

---

### 10. Unnecessary Reference Parameters

**Files**: Multiple (`Actions.php:25`, `Manage.php:24`, etc.)

```php
public function __construct(CodecheckPlugin &$plugin) // & is unnecessary
```

PHP objects are already passed by reference. The `&` is misleading.

**Fix**: Remove all `&` from object parameters

---

### 11. Incomplete Down Migration

**Files**: `CodecheckSchemaMigration.php:105-108`

```php
public function down(): void {
Schema::drop('codecheck_metadata');
// Missing: Delete created genres
}
```

**Fix**: Add genre cleanup to down migration

---

### 12. Certificate Script Not Integrated

**Files**: `classes/RetrieveReserveIdentifiers/CertificateRetrievingAndCreation.php`

This is a standalone CLI script that requires `.env` file (not documented), uses `stdin` for input, and is not integrated into plugin workflow.

**Fix**: Refactor into service class usable by plugin forms

---

## 🟢 LOW PRIORITY ISSUES

### 13. Inline Styles in JavaScript

**Files**: `js/submission-fields.js` (multiple locations)

```javascript
style="background: #006798; font-size:.875rem; ..."
```

**Fix**: Use CSS classes from stylesheet

---

### 14. No Input Validation

**Files**: Form handlers

Repository URLs, manifest filenames, etc. have no validation for:
- URL format
- XSS attempts
- Path traversal
- Reasonable length limits

**Fix**: Add validation in form handlers and JavaScript

---

### 15. Missing Internationalization

**Files**: JavaScript and some PHP

```javascript
+ Add File // Hard-coded English
```

**Fix**: Use OJS translation system for all user-facing strings

---

### 16. Empty CSS File

**Files**: `css/codecheck.css`

Plugin loads CSS file but it's empty. All styling is either inline or missing.

**Fix**: Move inline styles to CSS file or remove the empty file

---

### 17. No Error Handling in JavaScript

**Files**: `js/submission-fields.js`

All operations assume success. No try-catch blocks, no validation feedback to users.

**Fix**: Add error handling and user feedback

---

### 18. Incomplete Test Coverage

**Files**: `cypress/tests/functional/CodecheckPlugin.cy.js`

Most tests are TODO comments. No unit tests exist.

**Fix**: Implement test suite

---

## 📋 Recommended Implementation Plan

### Phase 1: Critical Fixes (Immediate)
- [ ] Fix data storage architecture - Remove unused DAO or implement sync
- [ ] Remove hard-coded genre IDs - Fix validation logic
- [ ] Remove dead code - Clean up unused methods/classes
- [ ] Fix SQL query - Use query builder
- [ ] Add missing Exception import

### Phase 2: High Priority (Next Sprint)
- [ ] Refactor main plugin file into smaller classes
- [ ] Standardize manifest file data format
- [ ] Add proper error handling
- [ ] Implement missing validation
- [ ] Remove unnecessary reference parameters
- [ ] Fix genre creation race condition
- [ ] Add debug mode toggle for logging

### Phase 3: Quality Improvements (Next Version)
- [ ] Integrate certificate management into plugin
- [ ] Move all strings to localization files
- [ ] Create comprehensive CSS
- [ ] Implement down migration fully
- [ ] Complete test coverage

---

## Suggested Architecture Refactoring

```
CodecheckPlugin.php (< 150 lines)
├─ registerHooks()
└─ loadComponents()

classes/
├── Handlers/
│ ├── SubmissionFormHandler.php
│ ├── ReviewDisplayHandler.php
│ └── ArticleDisplayHandler.php
├── Services/
│ ├── CodecheckDataService.php
│ ├── CertificateService.php
│ └── GenreService.php
├── Validators/
│ ├── SubmissionValidator.php
│ └── FileValidator.php
└── [existing folders...]
```

This would improve maintainability, testability, and follow OJS plugin best practices.

---

**Analysis performed**: 2025-11-03
**OJS Version**: 3.5.x
**PHP Version**: 8.2.0+