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

[DS-97] Instructor Reference field is not displayed on Schedule/Teasers #13

Merged
merged 9 commits into from
Oct 25, 2022

Conversation

froboy
Copy link

@froboy froboy commented Oct 18, 2022

Related Issue/Ticket:

PLEASE CHECK BASE BRANCH FOR YOUR PR
ONLY urgent and approved fixes for point release should go to master branch

https://yusa.atlassian.net/browse/DS-97

Instructor data from field_gc_instructor_reference was not previously displaying on event teasers or the VY schedule page, as per #12. With this change, Instructors from that field will be used in the display and if the field is not populated any data in the deprecated Host's Name field will be used.

Inheritance should go as follows:

  • display instance Instructor if set, else
  • display series Instructor if set, else
  • display instance Host if set, else
  • display series Host, else
  • display nothing

Steps to test:

  • Before testing ensure dependencies are installed. This work depends on a new patch for field_inheritance that's added.
  • In order to get data into the Host's Name field you may need to re-enable the field at
    admin/structure/events/instance/types/eventinstance_type/virtual_meeting/edit/form-display
    admin/structure/events/instance/types/eventinstance_type/live_stream/edit/form-display
    admin/structure/events/series/types/eventseries_type/virtual_meeting/edit/form-display
    admin/structure/events/series/types/eventseries_type/live_stream/edit/form-display
  • Add a recurring Live Stream and Virtual Meeting
  • Observe the Instructor displayed on the Event Teaser (in the "Up Next" section or on the old-style homepage) and the Schedule page displays the proper content as per the inheritance above.

Virtual_YMCA___Drush_Site-Install
Virtual_YMCA___Drush_Site-Install

Quality checks:

Please check these boxes to confirm this PR covers the following cases:

  • Maintaining our upgrade path is essential. Check one or the other:
    • This PR provides updates via hook_update_N or other means.
    • No updates are necessary for this change.
  • Front end fixes should be tested against all of the Open Y Themes.
    • Tested against Carnation
    • Tested against Lily
    • Tested against Rose
    • This change does not contain front-end fixes.
  • I have flagged this PR "Needs Review" or pinged the VY devs/QA
    team in Slack

@froboy froboy marked this pull request as ready for review October 24, 2022 21:39
Copy link

@podarok podarok left a comment

Choose a reason for hiding this comment

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

Some important changes needed

@@ -8,6 +8,7 @@
use Drupal\Core\Datetime\DrupalDateTime;
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\openy_gated_content\VirtualYAccessTrait;
use Drupal\taxonomy\Entity\Term;
Copy link

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

This use was outdated and was replaced with the entity queries. I've removed it instead.

public function getInheritedFieldLabels(int $id, string $field): array
{
// Get the field values from the parent series.
$series_values = \Drupal::service('entity_type.manager')
Copy link

Choose a reason for hiding this comment

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

Let's use Dependency Injection and avoid static calls for better container performance and cacheability

->{$field}
->referencedEntities();
// Get the field values from the instance.
$instance_values = \Drupal::service('entity_type.manager')
Copy link

Choose a reason for hiding this comment

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

Let's use Dependency Injection and avoid static calls for better container performance and cacheability

@froboy froboy requested a review from podarok October 25, 2022 14:42
@podarok podarok merged commit 5b64b6c into YCloudYUSA:master Oct 25, 2022
@froboy froboy deleted the event-series-instructor branch November 7, 2022 16:25
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.

2 participants