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

[3.3] 1924 export associations as resource relationship #1933

Open
wants to merge 39 commits into
base: Development
Choose a base branch
from

Conversation

Atticus29
Copy link
Collaborator

@Atticus29 Atticus29 commented Dec 4, 2024

Description

This PR allows for the download of related records. It addresses issue #1924 .

From the issue specs:

Adds DwcArchiverResourceRelationship class (following format similar to DwcArchiverIdentifier, DwcArchiverMaterialSample, and DwcArchiverAttribute
Add include Resource Relationship extension checkbox to all download interfaces and web services
Currently External and Internal Associations are concatenated into associatedOccurrences within occurrences.csv core file.
associatedOccurrences will be removed from DwC-Archive export (simply comment out lines 137 and 138 within DwcArchiverOccurrence). We might remove this column from the omoccurrences schema, but let's just comment it out for now.
Within DwcArchiverOccurrence getAssociationStr and getAssociationJSON will be completely removed.
appendSpecimenDuplicateAssociations function will be removed with functionality integrated as a batch function, similar to functions called between lines 1821 and 1827 within DwcArchiverCore
Currently observation associations are appended into the associatedTaxa field, which needs to be removed. The associatedTaxa field will remain in the DwC Archive export, but should now only contain verbatim text from the associatedTaxa field.

QA Notes

Live on dev001. In addition to all of the nuances of what we should map from Symbiota Native fields to DwC, note that I haven't tested the Darwin Core-only download or skipping the download entirely in a while.

Notes/Known Issues

This PR does not accommodate external or observational associations. They are in a separate issue.

Pull Request Checklist:

Pre-Approval

  • There is a description section in the pull request that details what the proposed changes do. It can be very brief if need be, but it ought to exist.
  • Hotfixes should be branched off of the master branch and PR'd using the merge option (not squashed) into the hotfix branch.
    • N/A
  • Features and backlog bugs should be merged into the Development branch, NOT master
  • All new text is preferably internationalized (i.e., no end-user-visible text is hard-coded on the PHP pages), and the spreadsheet tracking internationalizations has been updated either with a new row or with checkmarks to existing rows.
    • N/A
  • There are no linter errors
  • New features have responsive design (i.e., look aesthetically pleasing both full screen and with small or mobile screens)
    • N/A
  • Symbiota coding standards have been followed
  • If any files have been reformatted (e.g., by an autoformatter), the reformat is its own, separate commit in the PR
    • If so, noted in code diff
  • Comment which GitHub issue(s), if any does this PR address
  • If this PR makes any changes that would require additional configuration of any Symbiota portals outside of the files tracked in this repository, make sure that those changes are detailed in this document.
    • N/A

Post-Approval

  • It is the code author's responsibility to merge their own pull request after it has been approved
  • If this PR represents a merge into the Development branch, remember to use the squash & merge option
  • If this PR represents a merge into the hotfix branch, remember to use the merge option (i.e., no squash).
  • If this PR represents a merge from the Development branch into the master branch, remember to use the merge option
  • If this PR represents a merge from the hotfix branch into the master branch use the squash & merge option
    • a subsequent PR from master into Development should be made with the merge option (i.e., no squash).
    • Immediately delete the hotfix branch and create a new hotfix branch
    • increment the Symbiota version number in the symbase.php file and commit to the hotfix branch.
  • If the dev team has agreed that this PR represents the last PR going into the Development branch before a tagged release (i.e., before an imminent merge into the master branch), make sure to notify the team and lock the Development branch to prevent accidental merges while QA takes place. Follow the release protocol here.
  • Don't forget to delete your feature branch upon merge. Ignore this step as required.

Thanks for contributing and keeping it clean!

@themerekat
Copy link
Collaborator

Change verbiage here from "include Associated Records" to "include Resource Relationships (associations & linked resources)"
image

"request": "launch",
"port": 9003,
"xdebugSettings": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file was autoformatted. But also the xdebugSettings section was added because my xDebugger values were being very confusingly truncated. Fixed here slash not at all sure why this is being tracked in the repo.

@Atticus29
Copy link
Collaborator Author

include Resource Relationships (associations & linked resources)

Fixed in 20a3b08.

Atticus29 and others added 5 commits December 10, 2024 14:03
… omoccurrences so that occurrenceID can be queried
- Load associations via 2 separate queries:
-- one using occid
-- the other using occidAssociate with the relationship translated to its inverse
$termArr['deleteMeOccidAssociate'] = 'https://dwc.tdwg.org/terms/#dwc:resourceRelationshipID';
$columnArr['deleteMeOccidAssociate'] = 'oa.occidAssociate';
$termArr['resourceRelationshipID'] = 'https://dwc.tdwg.org/terms/#dwc:resourceRelationshipID';
// $columnArr['resourceRelationshipID'] = 'IFNULL(IFNULL(IFNULL(oa.objectID, o.occurrenceID), o.recordID), oa.resourceUrl)';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll delete some of these commented-out lines once we're more confident about the current strategy.

return array_diff_key($dataArr, array_flip($trimArr));
}

// @TODO decide if setDynamicFields is needed
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so, right?

foreach($this->fieldArr['fields'] as $colName){
if($colName) $sqlFrag .= ', ' . $colName;
}
// $this->sqlBase = 'SELECT ' . trim($sqlFrag, ', ') . ' FROM omoccurassociations ';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will remove upon approval.

</div>
<div class="formField-div">
<input name="collid" type="hidden" value="<?= $collid ?>" >
<input name="MAX_FILE_SIZE" type="hidden" value="10000000" />
<button name="submitAction" type="submit" value="initiateImport"><?= $LANG['INITIALIZE_IMPORT'] ?></button>
<button name="submitAction" class="submit__button--no-left-margin" type="submit" value="initiateImport"><?= $LANG['INITIALIZE_IMPORT'] ?></button>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Little styling fix

@@ -8,7 +8,7 @@
else include_once($SERVER_ROOT . '/content/lang/collections/datasets/datapublisher.en.php');
header('Content-Type: text/html; charset=' . $CHARSET);

$collid = array_key_exists('collid', $_REQUEST) ? filter_var($_REQUEST['collid'], FILTER_SANITIZE_NUMBER_INT) : 0;
$collid = array_key_exists('collid', $_REQUEST) ? filter_var($_REQUEST['collid'], FILTER_SANITIZE_NUMBER_INT) : 0; // @TODO collid is really coming from db in the searchvar attribute of the request, right? So it'll always be incorrect here?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe a future easy issue to clean up?

@@ -337,11 +338,14 @@ function importTypeChanged(selectElement){
<option value="externalOccurrence"><?= $LANG['EXTERNAL_OCCURRENCE'] ?></option>
<option value="observational"><?= $LANG['OBSERVATION'] ?></option>
</select>
<div class="top-breathing-room-rel danger" style="color: var(--danger-color);">
<caption><?= $LANG['ASSOCIATION_UPLOAD_WARNING'] ?></caption>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A little behavioral nudge for the end users who could footgun themselves with bulk uploading associations matching to the correct catalogNumber in an off-target collection (since cross-collection associations are correctly allowed as well).

@@ -33,7 +33,7 @@
$LANG['FIELD_MAPPING'] = 'Field Mapping';
$LANG['BATCH_DELETE'] = 'Batch Delete';
$LANG['NEW_BLANK_RECORD'] = 'Link media to new blank record if catalog number does not exist';
$LANG['MATCHING_IDENTIFIERS'] = 'Update or delete records with matching identifiers (subject identifier plus objectID or resourceUrl)';
$LANG['MATCHING_IDENTIFIERS'] = 'Update or delete existing association with matching identifiers (subject identifier plus objectID or resourceUrl)';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Per conversations with @themerekat , minor copy adjustments here.

@Atticus29 Atticus29 marked this pull request as ready for review January 22, 2025 20:31
@Atticus29 Atticus29 requested review from egbot and themerekat January 22, 2025 20:31
@Atticus29 Atticus29 changed the title 1924 export associations as resource relationship [3.3] 1924 export associations as resource relationship Jan 22, 2025
@themerekat
Copy link
Collaborator

I think something is going slightly wrong with the exporter's verbatimScientificName (not sure if this is upon export or during the creation of the association). The "hostOf" relationship should be with Apidae

From the export:
image

From the public display:
image

@Atticus29
Copy link
Collaborator Author

I think something is going slightly wrong with the exporter's verbatimScientificName (not sure if this is upon export or during the creation of the association). The "hostOf" relationship should be with Apidae

From the export: image

From the public display: image

Oh interesting. This is an observational association. Maybe we make that a problem for FutureMark?

* swap the occid and occidAssociate in getAssociatedRecords in classes/AssociationManager.php in order to get any results in the external observation export

* add third logic block to getAssociatedRecords targetting observations and external observations

* begin to remove cruft on 1924-pt2-external-and-observational-occurrences branch

* remove more cruft

* remove more cruft

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

Successfully merging this pull request may close these issues.

3 participants