fix(ep_approval): Fix error handling, add test for single record flow - #917
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
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?
There was a problem hiding this comment.
Yes, I mis-read, sorry!
But maybe we can have some kind of warning or log with the return, because the main bug might still happen if this component gets triggered by the system user either via a script or separately maybe due to a support ticket, and we don't return silently. WDYT?
There was a problem hiding this comment.
wasn't a test added to test the fix of the initial bug?
There was a problem hiding this comment.
I missed adding it before, we should add it.
| 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!
374fe8c to
7803c6b
Compare
| apprn = [{"scheme": "apprn", "identifier": rn} for rn in reportnumbers] | ||
| new_identifiers = apprn + not_apprn | ||
| else: | ||
| new_identifiers = not_apprn |
There was a problem hiding this comment.
if the record does not have a committee_approval field then the identifiers are getting stripped out. That means, we need in the migration to populate the custom_field. This will be taken into account when running the https://gitlab.cern.ch/cds-team/production_scripts/-/merge_requests/59 and during migration.
There was a problem hiding this comment.
Shall we raise a validation error or warning when for some reason the committee_approval field is missing but apprn identifiers were passed instead of stripping them out? @kpsherva @palkerecsenyi @sakshamarora1
|
- Fix error handling and add test for single-record flow. - Add back cdsrn validation on identifiers. - Change committee_approval.reportnumber from a string to a list of strings; it is now the single source of truth for apprn metadata identifiers on every edit/publish via _regenerate_apprn_identifier. - Accept action writes [report_number]; views.py passes the list through and joins it for inclusion-request comments. - CommitteeApproval.js and RecordVersionItem.js render the list joined with ", "; step guards use .length. - detail.html collects all apprn identifiers via selectattr/join. - Revert incorrect draftRecordId !== record.id guard from CommitteeApproval.js. - invenio.cfg: use always_valid for apprn so migrated identifiers are not rejected on subsequent edits. - Fix pre-existing test failures: grant origin format (uuid → pid_version_index) and wrong referee-grant scope assertions. Co-Authored-By: Saksham <sakshamarora1001@gmail.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
7803c6b to
6512f62
Compare
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