feat(deposition): always add timestamp to assemblyname and get test param directly from config

[anna-parker]

This will prevent us getting into the state we had in pathoplexus/ena-submission#235 where the pod is unable to retry submissions and I need to resubmit them manually.

Screenshot

PR Checklist

• All necessary documentation has been adapted.
• The implemented feature is covered by appropriate, automated tests.
• Any manual testing that has been done is documented (i.e. what exactly was tested?) -> I also double checked we still submit to dev for the tests after this change by going on https://www.ebi.ac.uk/ena/submit/webin/ for the test account

🚀 Preview: Add preview label to enable

[claude]

Claude finished @anna-parker's task in 1m 43s —— View job

---

Code Review

• Gather context and read changed files
• Review code changes
• Post review feedback

---

The PR correctly addresses the retry failure issue by always embedding a timestamp+entropy in assembly names, ensuring ENA never rejects a retry submission due to a duplicate name. The refactor to read config.test directly (instead of passing it as a parameter) is also cleaner and eliminates a footgun where the old test=True default in create_ena_assembly/post_webin_cli could silently cause issues.

A few observations below.

Minor issues:

1. Misleading comments in integration tests — after removing the test= parameter, three call sites in test_ena_submission_integration.py still have the comment:

   # IMPORTANT: set test=true below or this script may submit sequences to ENA prod
   assembly_table_create(db_config, config)

   The comment says "set test=true below" but there's no longer any parameter to set — it's now controlled entirely by config.test. The comment should be updated to e.g. "IMPORTANT: ensure config.test=True or this script may submit sequences to ENA prod".

2. Test coverage regression — the old test_create_manifest used a single assertEqual(data, expected_data) dict comparison, which would have caught any unexpected new key appearing in the manifest output. The replacement with individual per-key assertions drops that coverage. The ASSEMBLYNAME check (assertTrue(data["ASSEMBLYNAME"].startswith("LOC_0001TLY.1"))) is necessary and correct, but it would be worth adding a check on the total number of expected keys to keep the coverage intact, e.g.:

   expected_keys = {"STUDY", "SAMPLE", "ASSEMBLYNAME", "ASSEMBLY_TYPE", "COVERAGE",
                    "PROGRAM", "PLATFORM", "DESCRIPTION", "MOLECULETYPE", "AUTHORS", "ADDRESS"}
   self.assertEqual(set(data.keys()), expected_keys)

The old dangerous default is now gone — create_ena_assembly(..., test=True) and post_webin_cli(..., test=True) previously defaulted to the dev endpoint, which was surprising to readers and could mask bugs. Reading from config.test is strictly better.

Overall this is a clean, well-scoped fix. The two items above are non-blocking suggestions.

[theosanderson]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. More of your lovely PRs please.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".