Fix datasource list status for known engines#159
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
EntelligenceAI PR SummaryFixes incorrect 'saved' status display for datasources with a defined engine and non-'choice' auth method when required fields are missing.
Confidence Score: 5/5 - Safe to MergeSafe to merge — this PR fixes datasource list status handling for known engines and has been assessed with no identified issues, logic concerns, or security problems. The automated review found the code to be clean with zero critical, significant, or medium-severity findings. No pre-existing unresolved comments are present, and the change appears well-scoped to its stated purpose of correcting status reporting for known database engines. Key Findings:
|
|
Additional context for reviewers: This is a small display/status fix in What changed:
This is intentionally limited to the status calculation in |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
c96c565 to
3972c70
Compare
| assert reg.get("mysql") is not None | ||
| assert reg.get("mysql").display_name == "MySQL" | ||
|
|
||
| def test_builtin_optima_api_template(self): |
There was a problem hiding this comment.
The existing tests are testing how the registry parses templates, not whether a specific template has the right values. But the test_builtin_optima_api_template test is just checking that the YAML was typed correctly, which isn't something we test for other datasources. Rather, if every new datasource added a test like this, the file would grow a lot without real value, because the file already gets validated by test_validate_file_returns_engines and test_all_engines_sorted.
Could you remove this test?
There was a problem hiding this comment.
Done, removed test_builtin_optima_api_template. The generic datasource registry/parser tests remain in place.
There was a problem hiding this comment.
Thanks. I see that you removed the datasource template from the datasources.md file as well, although I suggested removing just from the tests.
There was a problem hiding this comment.
You are right, I removed it from datasources.md as well to keep this PR focused only on the status handling fix.
For context, that datasource is for connecting to an API I wrote for Comarch Optima, which is a popular ERP system in Poland. If you think this datasource belongs in the built-in sources, I can add it back in a separate, dedicated PR with the relevant context.
There was a problem hiding this comment.
That sounds good. Sure, please feel free to create a new PR adding the template for this datasource.
5d5d1f5 to
02a1c36
Compare
Add the WebArm Comarch Optima API built-in datasource template and cover its parsed metadata in datasource tests. Keep known-engine status handling ahead of the generic empty-fields fallback so known engines report saved or incomplete correctly.
02a1c36 to
e218f27
Compare
When an engine definition is available, validate required fields before treating an empty credentials map as incomplete. This lets known engines without required fields appear as saved instead of incomplete.