Refactor metadata PO files generation

[iangmaia]

Fixes AINFRA-1724
See discussion on Slack: p1767962906955309-slack-CC7L49W13
See test PR: woocommerce/woocommerce-ios#16502

Summary

The main goal of this PR is to fix the issue when generating the .po / .pot files where new entries aren't present in the output or removed entries are removed. To do that, I've consolidated the metadata PO file generation into a new PoFileGenerator class using gettext. The ios_update_metadata_source and an_update_metadata_source actions are now deprecated and delegate to gp_update_metadata_source.

What's Changed

New Features:

• Translator comments: source_files now accepts { path: 'file.txt', comment: 'translators: ...' } format
• commit_changes option: Auto-commit support on gp_update_metadata_source

Output Changes:

• Entries sorted alphabetically by msgctxt for deterministic output
• Translator comments (#. lines) must now be explicitly provided—they're no longer preserved from existing PO files
• PO-Revision-Date is now updated with the current date time

Breaking Changes

1. Entry order will change in generated PO files
2. Translator comments will be lost unless migrated to the new format:

# Before
source_files: { app_store_subtitle: 'path/to/subtitle.txt' }

# After (to preserve comments)
source_files: {
  app_store_subtitle: {
    path: 'path/to/subtitle.txt',
    comment: 'translators: Limit to 30 characters!'
  }
}

Migration

When upgrading, callers need to copy comments from .po/.pot files to the gp_update_metadata_source call site in the Fastfile to make sure the comments remain in the generated file. See MIGRATION.md for details.

Deprecations

Old Action	Replacement
ios_update_metadata_source	gp_update_metadata_source with commit_changes: true
an_update_metadata_source	gp_update_metadata_source

Deprecated actions still work (API unchanged) but the output will be different (the comments and header will be removed and the keys will be sorted alphabetically, mainly).

[dangermattic]

	1 Warning
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

[AliSoftware]

I've only skimmed the diff so far (so not a deep dive review, and also haven't tested it at all) so this is only a first-pass review. Still promising though!

[AliSoftware]

We should avoid shelling out to do git operations

• First and most importantly, calling sh("git add #{file}") is unsafe and a security risk (think paths with spaces, accidental extra shell interpolation, injection …). If we still wanted to shell out to call git at the very least we should call this as sh('git', 'add', file) (instead of sh("git add #{file}")) to avoid the extra layer of having the shell parsing and interpreting the string as a command and separate arguments but instead pass the separate arguments from the get go
• But also, there are already some fastlane actions that exist to handle git_add and similar, so would probably make more sense to use them
• Or alternatively use one of our git helpers for that

[iangmaia]

Good point -- this code was moved from the iOS action while consolidating iOS and Android into it and ended up unchanged. Updated to use Fastlane actions on 4b92f8f.

[AliSoftware]

Similarly, we should call the existing ensure_git_repo_clean action (or whatever it's called) here instead of reinventing the wheel

[iangmaia]

Updated on 4b92f8f.

[AliSoftware]

I'm not sure it's worth for the helper (and the action that calls it) to need to read / rely on the existing po file anymore (after all the whole point of this new approach is to not have to rely on an existing po to begin with)

Instead I'd recommend to remove the @existing_po_path attribute from this helper entirely, and instead hardcode the typical header fields and values we want to include in the PO files generated from scratch

[iangmaia]

I also went back and forth with this but I agree: aside from making the PO file structure more predictable, not worrying about a previously existing file simplifies the code a lot. Updated on cede46f.

[AliSoftware]

Code Review: PR #684 - Refactor metadata PO files generation

I'll provide a comprehensive review of this pull request, analyzing the refactoring, implementation quality, and potential concerns.

★ Insight ─────────────────────────────────────
This PR represents a significant architectural shift from a stateful stream-processing approach (MetadataBlock classes that processed PO files line-by-line) to a declarative generation approach (PoFileGenerator that builds PO files from scratch). This is a classic refactoring pattern that trades incremental updates for simplicity and determinism.
─────────────────────────────────────────────────

🎯 Overall Assessment: STRONG REFACTOR ✅

This is a well-executed refactoring that significantly improves code maintainability. The PR demonstrates good engineering practices with comprehensive tests, clear migration documentation, and thoughtful API design.

---

✅ Strengths

1. Architecture Improvements

• Simplification: Removes 5 complex MetadataBlock classes (~245 lines) and replaces with a single, focused PoFileGenerator (~175 lines)
• Determinism: Alphabetically sorted entries ensure consistent output across runs
• Proper library usage: Leverages the gettext gem instead of manual PO file parsing/generation

2. API Design

# Clean support for both simple and complex formats
source_files: {
  app_name: 'path/to/name.txt',  # Simple
  app_store_subtitle: {           # With translator comment
    path: 'path/to/subtitle.txt',
    comment: 'translators: Limit to 30 characters!'
  }
}

This is elegant backward-compatible design.

3. Test Coverage

The new po_file_generator_spec.rb is exceptional (466 lines of comprehensive tests):

• Tests all entry types (standard, whats_new, release_note, release_note_short)
• Tests header generation
• Tests translator comments
• Tests entry ordering
• Tests that generated PO files can be parsed by gettext

4. Documentation

• Excellent MIGRATION.md with clear before/after examples
• Updated CHANGELOG.md with breaking changes clearly called out
• Deprecation warnings in the deprecated actions

---

⚠️ Issues & Concerns

1. Critical: Silent Data Loss Risk 🔴

In lib/fastlane/plugin/wpmreleasetoolkit/actions/common/gp_update_metadata_source.rb:426-432:

other_action.ensure_git_status_clean if params[:commit_changes]

Problem: The action regenerates the PO file from scratch but only checks git status if commit_changes: true. If called without commit_changes, existing translator comments are silently lost without warning.

Recommendation:

def self.run(params)
  UI.message "PO file path: #{params[:po_file_path]}"
  UI.message "Release version: #{params[:release_version]}"
  
  # Warn if we're about to overwrite an existing PO file
  if File.exist?(params[:po_file_path])
    UI.important("⚠️  Regenerating #{params[:po_file_path]} - any existing translator comments will be lost unless specified in source_files")
  end
  
  # ... rest of the method
end

2. Potential Issue: Missing File Validation 🟡

In lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata/po_file_generator.rb:822-834:

def extract_path_and_comment(value)
  case value
  when String
    [value, nil]
  when Hash
    [value[:path], value[:comment]]
  else
    raise ArgumentError, "Invalid source_files value: expected String or Hash, got #{value.class}"
  end
end

Problem: If a Hash is provided without a :path key, the code will return [nil, value[:comment]], leading to a confusing error later when trying to read the file.

Recommendation:

when Hash
  UI.user_error!("Hash must contain :path key, got: #{value.keys}") unless value.key?(:path)
  [value[:path], value[:comment]]

3. Code Smell: Inconsistent Verification 🟡

In lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_update_metadata_source.rb:672-679:

The deprecated iOS action removes the file existence check:

# Old verification:
UI.user_error!("Couldn't find file at path '#{value}'") unless File.exist?(value)

# New verification (removed):
# No file existence check!

But the Android action at line 363 keeps the verification. This is inconsistent. Since both now delegate to gp_update_metadata_source, both should have consistent behavior.

Recommendation: Keep the verification but update the error message to be accurate for generation (not just update).

4. Version Parsing Fragility 🟡

In lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata/po_file_generator.rb:895-898:

def parse_version(version)
  parts = version.split('.')
  [Integer(parts[0]), Integer(parts[1])]
end

Problems:

• No validation that parts[0] and parts[1] exist
• Integer() will raise ArgumentError for invalid input
• No handling of semver versions like "1.2.3"

Recommendation:

def parse_version(version)
  parts = version.split('.')
  UI.user_error!("Invalid version format '#{version}': expected 'X.Y'") if parts.length < 2
  
  begin
    [Integer(parts[0]), Integer(parts[1])]
  rescue ArgumentError
    UI.user_error!("Invalid version format '#{version}': major and minor must be integers")
  end
end

5. Test Gap: Empty whats_new Content 🟡

The tests cover empty release_note_short content (line 1406-1420) but not empty whats_new content. The create_whats_new_entry method doesn't check for empty content like create_release_note_short_entries does.

Recommendation: Add test and possibly skip empty whats_new entries:

def create_whats_new_entry(content, comment = nil)
  return nil if content.strip.empty?
  # ... rest of method
end

Then filter nils in create_entries_for_key.

6. Minor: Gemspec Changes Scope Creep 🔵

In fastlane-plugin-wpmreleasetoolkit.gemspec:188-195:

-  spec.name          = 'fastlane-plugin-wpmreleasetoolkit'
+  spec.name          = Fastlane::Wpmreleasetoolkit::NAME
...
-  spec.summary       = 'GitHub helper functions'
+  spec.summary       = 'Fastlane plugin for release automation'

These changes are good improvements but are technically unrelated to the PO file refactoring. Consider mentioning in the PR description why these were included.

---

🔍 Detailed Code Analysis

★ Insight ─────────────────────────────────────
The new PoFileGenerator class uses composition over inheritance (creating POEntry objects) rather than the old approach's template method pattern (StandardMetadataBlock subclasses). This is the right choice because PO entry generation has clear, distinct cases rather than shared behavior with variations.
─────────────────────────────────────────────────

PoFileGenerator Class (lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata/po_file_generator.rb:733-900)

Positives:

• Clean separation of concerns: header generation, entry creation, sorting, writing
• Good use of Ruby idioms (case/when, symbol keys, sort_by)
• Comprehensive documentation with examples
• Proper handling of multiline content

Code Quality Notes:

1. Line 772-789 - The sorting approach is excellent:

po.order = :none  # Disable GetText's sorting
entries.sort_by(&:msgctxt).each do |entry|  # Custom sort
  po[entry.msgctxt, entry.msgid] = entry
end

2. Line 849-851 - Good handling of rstrip to avoid trailing whitespace:

def create_standard_entry(msgctxt, content, comment = nil)
  create_entry(msgctxt, content.rstrip, comment)
end

3. Line 860-864 - The release note version formatting is clever:

msgid = "#{@release_version}:\n#{content}"
msgid = "#{msgid}\n" unless msgid.end_with?("\n")

---

📋 Migration Risk Assessment

Risk Level: MEDIUM 🟡

Breaking Changes:

1. ✅ Well documented in MIGRATION.md
2. ✅ Deprecated actions still work (delegate to new implementation)
3. ⚠️ Translator comments will be lost - this requires manual migration
4. ⚠️ Entry order changes - could break brittle tests

Mitigation Strategies:

• The deprecation approach (delegating to new implementation) is excellent
• Migration guide is clear and actionable
• Test PR mentioned (#16502) demonstrates validation in real project

Recommendation: Consider adding a pre-migration validation script that:

1. Scans existing .po files for translator comments (#. lines)
2. Reports which comments will be lost
3. Generates suggested source_files hash format with comments

---

🧪 Test Quality Analysis

The test suite is exemplary. Highlights:

1. Comprehensive coverage of all entry types
2. Tests both old and new API formats (String vs Hash in source_files)
3. Tests edge cases (empty content, trailing whitespace, multiline)
4. Validates generated output can be parsed by gettext library
5. Proper test isolation using in_tmp_dir

One gap: No integration test that validates the deprecated actions still produce reasonable output. The shared examples were updated but don't verify that old callers get expected results.

---

🔒 Security Considerations

✅ No security concerns identified

The code:

• Reads from file paths provided by the caller (expected behavior)
• Writes to file paths provided by the caller (expected behavior)
• Uses git operations safely through Fastlane actions
• No shell injection risks (all paths properly handled)
• No sensitive data exposure risks

---

📊 Performance Considerations

Improvement: The new approach should be faster because:

1. Builds PO from scratch (O(n)) vs. line-by-line processing with pattern matching (O(n*m))
2. Uses efficient gettext library instead of string manipulation
3. Single pass through source files instead of multiple passes

Potential concern: For very large PO files, building entirely in memory could be an issue, but metadata PO files are typically small.

---

✍️ Style & Maintainability

Excellent overall, with minor suggestions:

1. Line 802-814 - Consider extracting magic strings to constants:

MIME_VERSION = '1.0'
CONTENT_TYPE = 'text/plain; charset=UTF-8'
PLURAL_FORMS = 'nplurals=2; plural=n != 1;'

2. Line 885-893 - These three methods are very similar. Consider DRYing up:

def versioned_key_for(prefix, version)
  major, minor = parse_version(version)
  "#{prefix}_#{major.to_s.rjust(2, '0')}#{minor}"
end

def release_note_key_for_version(version)
  versioned_key_for('release_note', version)
end

---

🎯 Recommendations Summary

Must Fix 🔴

1. Add warning when overwriting existing PO files with potential comment loss
2. Validate Hash source_files entries have :path key
3. Add proper error handling to parse_version

Should Fix 🟡

1. Handle empty whats_new content consistently with release_note_short
2. Make verification consistent between iOS and Android actions
3. Add integration tests for deprecated actions

Nice to Have 🔵

1. Extract header constants
2. DRY up the versioned key methods
3. Create pre-migration validation script
4. Document why gemspec changes were included

---

✅ Final Verdict

APPROVE WITH MINOR CHANGES ⭐⭐⭐⭐

This is a high-quality refactoring that:

• ✅ Significantly improves code maintainability
• ✅ Has excellent test coverage
• ✅ Provides clear migration documentation
• ✅ Uses proper deprecation strategy
• ⚠️ Has a few edge cases that need addressing

The issues identified are all fixable and don't block approval. Address the critical warnings about data loss, and this will be an exemplary refactoring.

★ Insight ─────────────────────────────────────
This PR exemplifies good legacy code refactoring: it doesn't try to change everything at once, maintains backward compatibility through deprecation, provides migration docs, and includes comprehensive tests. The shift from parsing/transforming to generating is a textbook example of how introducing determinism (sorted output) can simplify complex code.
─────────────────────────────────────────────────

---

Great work overall! The codebase will be much more maintainable after this refactor. 🎉

[iangmaia]

Code Review: PR #684 - Refactor metadata PO files generation

Nice one!

---

The first point:

other_action.ensure_git_status_clean if params[:commit_changes]
Problem: The action regenerates the PO file from scratch but only checks git status if commit_changes: true. If called without commit_changes, existing translator comments are silently lost without warning.

This doesn't really apply as the idea is to overwrite the file and the check is just to be sure there's nothing else in progress.
This is a check I believe comes from the times when release managers were running the tools locally and is less relevant on CI. I think it's also nicer to just do it in the app release lanes instead of in the library. I'll just remove it.

---

But the Android action at line 363 keeps the verification. This is inconsistent. Since both now delegate to gp_update_metadata_source, both should have consistent behavior.

Interesting, this point is wrong (aside from the weird line reference? Perhaps it's the line number from the diff?). Both iOS and Android are almost identical. Perhaps it got confused with the diff? 🤔

---

About other recommendations, I've implemented:

1. Removed ensure_git_status_clean from gp_update_metadata_source.rb
2. Added :path key validation in extract_path_and_comment
3. Added proper error handling to parse_version
4. Empty whats_new content handling
5. DRYed up versioned key methods
6. Added a few new tests.

Updated on 073d704.