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

SDSS-1347: Auto-Person tagging from Workgroups #505

Open
wants to merge 8 commits into
base: 4.x
Choose a base branch
from

Conversation

ksharp-drupal
Copy link
Collaborator

READY FOR REVIEW

Summary

  • Enables tagging of imported Person nodes based on workgroup membership

Review By as soon as feasible

Criticality

  • Somewhat critical

Review Tasks

  1. Review new code module stanford-earth/stanford_person_tagging.git - note - was unable to add module to SU-SWS and include in composer.json so parked at stanford-earth
  2. Checkout this branch, run composer update, check that sunetid and workgroup tag fields added to person content type
  3. Run person importer, check that sunetid field in person type is populated
  4. Configure workgroup tagging at /admin/config/importers/person-tagging, associate sdss:faculty-regular with Faculty and sdss:faculty-affiliated with Faculty
  5. Check that Stanford Person Tagging has been added to cron jobs
  6. Test tagging either with /tag-persons-from-workgroups or by running cron job
  7. Check that person_tagging_wg fields are filled; news fields are not currently displayed

Associated Issues and/or People

https://stanfordits.atlassian.net/browse/SDSS-1347

@joegl joegl changed the title Sdss 1347 Person Tagging From Workgroups SDSS-1347: Auto-Person tagging from Workgroups Nov 15, 2024
@joegl
Copy link
Collaborator

joegl commented Nov 22, 2024

Looking really great! I've put together some requested changes and indicated whether they need to be done pre-merge or we can optimize is post-merge:

  • Rename two fields added to su_person node (before merge)
    • Rename su_person_wg_tags field to sdss_person_workgroup_tags (think we're actually not using this new field)
    • Rename su_person_sunetid field to sdss_person_sunetid
  • Move configuration into the module using the Drupal Form API (talk about before merge, can possibly do post-merge)
    • Remove the config_pages configuration and build the configuration form using the Drupal Form API.
    • Paragraph types shouldn't be used for capturing configuration data in a configuration form. Use a built in configuration form using the Drupal Form API
    • The role mappings form in the stanford_samlauth module is a good place to start: https://github.com/fourkitchens/stanford_samlauth/blob/1.x/src/Form/RoleMappingSettingsForm.php
    • I know these two are big requests so we can talk about best strategy for this in order to expedite
  • Store field references in a module settings config schema instead of hard-coded in the module (can be optimized post-merge)
    • Instead of having the workgroup_tags and sunetid fields hard-coded in the StanfordPersonTagging.php file, store and load them from configuration.
  • Add dependencies for modules it needs (before merge)
    • stanford_samlauth
  • I'd like to see it more "drupalized" rather than one method in one class (StanfordPersonTagging.php) doing all the work (can be optimized post-merge)
  • The .module code in the hook_cron could be improved (can be optimized post-merge)
  • I'm a little concerned about all the requests to the workgroup_api on every cron run – they could add up fast, especially if every person node is getting re-checked every cron run (every 4 hours). We should think about ways to reduce requests and cache date, or make requests more infrequent (think about before merge, can maybe optimize post-merge)
  • As discussed, auto-tag should use the existing person_type field on the su_person node, and we can ditch the new wg_tags field.

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

Successfully merging this pull request may close these issues.

2 participants