fix(ep_approval): Fix error handling, add test for single record flow - #917
fix(ep_approval): Fix error handling, add test for single record flow#917sakshamarora1 wants to merge 6 commits into
Conversation
70da679 to
8f82c43
Compare
| return False | ||
| # Migrated case: both flags point at the same version, so only the current version carries apprn | ||
| if approved_internal == source_internal: | ||
| return record["id"] == source_internal |
There was a problem hiding this comment.
If I understand correctly, that wil regenerate the apprn only for the accepted version. The truth is that all the newer versions should have the apprn number
86a2477 to
bba3e2e
Compare
The condition prevented the reviewed-version link from rendering when the draft and record share the same id, which is wrong for the single-record migration flow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- _regenerate_apprn_identifier now keeps any apprn entries already on the record (e.g. placed by the migration script), deduplicating by value and always placing the committee_approval reportnumber first. - Simplify the apprn lookup in detail.html using selectattr/first instead of a namespace + for-loop. - Switch apprn validator to always_valid so migrated identifiers are not rejected during subsequent edits. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
be22049 to
5f73073
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5f73073 to
45d3d5d
Compare
45d3d5d to
963cbe9
Compare
| pf["committee_approval"] = { | ||
| **ea, | ||
| "approved_public_version": new_record_id, | ||
| "source_public_version": src_id, |
There was a problem hiding this comment.
do we have documentation describing these keys?
There was a problem hiding this comment.
I will add some documentation in our internal docs
| def _validate_identifier_changes(self, identity, data, record, errors): | ||
| """Raise ValidationError if a non-system identity modifies apprn.""" | ||
| if identity.id == system_user_id: | ||
| return |
There was a problem hiding this comment.
Shouldn't this raise instead of return to keep inline with the docstring?
There was a problem hiding this comment.
but we return if the user is system. We raise for non-system users as per docstring, do I miss something?
| author_email = info@cds-rdm.com | ||
| year = 2022 | ||
| python_version = 3.9 | ||
| python_version = 3.14 |
There was a problem hiding this comment.
Is this leftover?
There was a problem hiding this comment.
I am not sure if we are still using that, but I upgraded locally to 3.14 and upgraded the information there too.
| "fastjsonschema<2.22", | ||
| "invenio-assets<4.2.5" | ||
| "invenio-assets<4.2.5", | ||
| "flask-caching>=2.3.0,<2.5.0" |
There was a problem hiding this comment.
Is it related to this PR?
There was a problem hiding this comment.
there is a separate commit pinning flask-cahcing, because tests were failing but it might be a leftover, I will check thanks!
3 participants
Fixes: #912
Also makes the code ready for the migration script: https://gitlab.cern.ch/cds-team/production_scripts/-/merge_requests/59
Related: #908