Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: 372: save dataset as xml in curation log #2112

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

alli83
Copy link
Collaborator

@alli83 alli83 commented Dec 1, 2024

Pull request for issue: ##372

This is a pull request for the following functionalities:

Datacite xml payload saved in log
Add unique constraint to identifier (Dataset model)

How to test?

Given that I am logged in as Admin
When I click Mint DOI button and refresh the page
Then DataCite XML is generated and saved in the curation log,

How have functionalities been implemented?

Any issues with implementation?

Any changes to automated tests?

acceptance test added

Any changes to documentation?

Any technical debt repayment?

Any improvements to CI/CD pipeline?

@alli83 alli83 force-pushed the feature-372-save-dataset-as-xml-in-log branch from a100155 to d18500d Compare December 3, 2024 02:14
@alli83 alli83 force-pushed the feature-372-save-dataset-as-xml-in-log branch from d18500d to defe6ab Compare December 12, 2024 03:46
Copy link
Contributor

@rija rija left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alli83,

praise: functionality works

issue: the type column in the curation log entry is currently "XML Saved", which is too vague (and the fact that its shows in the log implies that is saved, no need to spell that out)
suggestion: Use "Sent DataCite XML"

issue: protected/extensions/CTypeFormatter.php is missing comments explaining what it is for
suggestion: add PHPDoc block to the class with explanation on the reason it exists

issue: there are changes to the related identifiers which don't seem to be part of the PR's goal and they are not mentioned in any of the section of the PR description
suggestion: Add more info to the PR description about all the changes made in this PR

praise: all the tests are passing

note: @kencho51, @pli888 when deploying this to live production we need to remember to run DB migrations manually there, because I just noticed on AWS they are currently run only as part of databaseReset, which we cannot run on live production. I'll create a ticket to make the schema migration also a separate gitlab job before deploy going forward

@alli83 alli83 force-pushed the feature-372-save-dataset-as-xml-in-log branch from defe6ab to bef88e9 Compare December 16, 2024 22:02
Copy link
Contributor

@kencho51 kencho51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alli83 ,

praise: Can see the xml was generated and can be seen in the curation log comment column.

praise: The acceptance test is passing.

praise: The newly introduced JS code can collapse and expand the xml.

praise: The CTypeFormatter checks if the data contains XML-like data and then change the type to raw, which ensure it is displayed as raw HTML (or preformatted text) instead of being escaped or processed as plain text.

issue: There is no test for the new component class LogCurationFormatter.php.
suggestion: Better to have unit test for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants