Making WordPress.org

Opened 10 years ago

Closed 10 years ago

Last modified 4 weeks ago

#561 closed enhancement (worksforme)

Allowing filtering of speakers by track

Reported by: hlashbrooke's profile hlashbrooke Owned by: iandunn's profile iandunn
Milestone: Priority: normal
Component: WordCamp Site & Plugins Keywords: has-patch
Cc:

Description

As discussed with Ian Dunn over email.

Currently the [speakers] shortcode shows all the published speakers. It would be helpful if you could filter them by track so that, for example, you can display your regular session speakers in a different format/location to your workshop instructors.

My solution adds a new meta field to the wcb_speaker post type called _wcpt_speaker_tracks. This is processed and added to all speakers whenever a session is saved. I have then modified the [speakers] shortcode to allow for a track parameter that accepts a comma-separated list of track slugs so only speakers in those tracks are displayed. The parameter defaults to all. In addition, I have added each track slug as class to each .wcorg-speaker div so each track's speakers can be styled differently.

Attachments (3)

561.diff (4.4 KB) - added by hlashbrooke 10 years ago.
Solution as described in ticket body
561.2.diff (3.6 KB) - added by hlashbrooke 10 years ago.
Using multiple queries, but storing data in transient
561.3.diff (4.1 KB) - added by hlashbrooke 10 years ago.
Setting transient per shortcode

Download all attachments as: .zip

Change History (21)

@hlashbrooke
10 years ago

Solution as described in ticket body

#1 @iandunn
10 years ago

Thanks Hugh, that sounds pretty good. I'm curious about the new meta field, though. Doesn't the data needed to generate the list of speakers already exists in the _wcpt_speaker_id fields (plural) on the Session posts. It seems to me that it'd be simpler and more straightforward to use that, rather than duplicating it in a new field. What do you think?

#2 @hlashbrooke
10 years ago

The reason I used a new meta field is because with the [speakers] shortcode you're doing a query using the wcb_speaker post type and the _wcpt_speaker_id meta field is saved on the wcb_session posts, so I couldn't find an intuitive way to handle that with WP_Query. I added the new meta field to make the query easier so now the shortcode now just adds a meta_query to the existing query.

The other option would have been to make the shortcode query the wcb_session posts for the sessions in the specified tracks and then take the speakers from the post meta there. The problem with that is it would not have been possible to retain the ordering parameters that the [speakers] shortcode currently accepts. That's why I went for the method in the patch. If you're happy to can the ordering parameters then I can write a new patch that does things that way instead.

#3 @iandunn
10 years ago

What about something like this?

$speaker_ids    = array();
$track_sessions = get_posts( array(
        'post_type'   => 'wcb_session',
        'numberposts' => -1,
        'tax_query'   => array(
                array(
                        'taxonomy' => 'wcb_track',
                        'field'    => 'slug',
                        'terms'    => explode( ',', $track_slugs ),
                ),
        ),
) );

foreach ( $track_sessions as $session ) {
        $session_speaker_ids = get_post_meta( $session->ID, '_wcpt_speaker_id' );
        $speaker_ids         = array_merge( $speaker_ids, $session_speaker_ids );
}

$speakers = get_posts( array(
        'post_type'   => 'wcb_speaker',
        'numberposts' => -1,
        'post__in'    => $speaker_ids,

        // order, etc parameters can still go here
) );

#4 @hlashbrooke
10 years ago

Yeah - that would work. I was trying to avoid doing multiple queries like that though as it affects performance on the frontend, which is where it counts. If that's the way you would prefer though then I can submit a new patch (or you can just add it of course) - I'm on my phone right now, so can look at it again in the morning.

#5 @iandunn
10 years ago

Yeah, I figured we can cache the end result in a transient for an hour.

#6 @hlashbrooke
10 years ago

I've updated the patch to handle the queries like you suggested - it stores the array of speaker IDs in a transient for 1 hour and I've allowed a force refresh of the transient by adding a refresh parameter to the page URL.

@hlashbrooke
10 years ago

Using multiple queries, but storing data in transient

#7 @iandunn
10 years ago

Looks good :)

Instead of refreshing the transient via a URL parameter, I think we can do it automatically with something like this:

add_action( 'save_post', array( $this, 'refresh_wordcamp_speaker_ids_transient' ), 10, 2 );
public function refresh_wordcamp_speaker_ids_transient( $post, $post_raw ) {
        $relevant_post_types = array( 'wcb_speaker', 'wcb_session' );

        if ( has_shortcode( 'speakers', $post ) || in_array( $post->post_type, $relevant_post_types ) ) {
                delete_transient( 'wordcamp_speaker_ids' );
        }
}

What do you think?

Other than that, the only thing I'd mention is that we can use HOUR_IN_SECONDS instead of 3600, to make it a little more obvious.

Still waiting to hear back from Andrea, but I'll post an update when I do.

#8 @hlashbrooke
10 years ago

An issue just occurred to me here that would affect my use case. We plan on using [speakers] shortcode twice on the same page. The first instance would be [speakers track="talks"] and the second would be [speakers track="workshops"]. If we set a transient in the first instance then those same speaker IDs would be shown in the second instance, which would pretty much defeat the point of the whole thing. It would be the same if you used the shortcode on two different pages - moving from one page to the other would use the same set of speakers IDs.

I think, due to the nature of the data that is displayed, we could probably do away with the transient to be honest. WordCamps are generally 3 days long at the most (in most cases they're less), so the amount of speakers that will actually be shown isn't so drastic that the page load times will be hugely affected by doing two queries for each shortcode instance.

I'm happy to can the transient and will submit an updated patch if thats OK with you. Unless you can think of another way around the issue? If you could use wildcards in delete_transient() then it would be easy, but that's not possible at the moment.

#9 @iandunn
10 years ago

It should be easy to get around that by making sure the transient key is unique to the data, which is a best practice anyway. Something like:

$concatenated_data = $track_slugs . implode( ',', $speaker_ids );
$transient_key     = 'wordcamp_speaker_ids_' . substr( md5( $concatenated_data ), 0, 24 ); // transient keys are limited to 45 characters

Truncating the MD5 hash technically opens it up to collisions, but it's probably rare enough that it won't be an issue in reality.

#10 @hlashbrooke
10 years ago

I was thinking of something like that - the only issue is that it doesn't give you a way to force refresh the transient, but having thought about it I don't think that's entirely necessary to be honest seeing as though it will refresh every hour anyway.

Adding an updated patch below.

@hlashbrooke
10 years ago

Setting transient per shortcode

#11 @iandunn
10 years ago

  • Owner set to iandunn
  • Resolution set to fixed
  • Status changed from new to closed

In 779:

WordCamp Post Types: Add 'track' parameter to [speakers] shortcode.

The id attribute on each speaker div is now deprecated, because the shortcode now supports use cases where the
shortcode is used multiple times on a single page, and assigning duplicate IDs violates the HTML spec.

Fixes #561
props hlashbrooke

#12 follow-up: @iandunn
10 years ago

Sorry the for delay Hugh, but this is committed now. Thanks for the patch :)

Let me know how it works out when you use it on your site.

#13 @hlashbrooke
10 years ago

Awesome - thanks! I've been on leave for the past week, but I'm going to be looking into this again this week and will let you know if I encouter any issues.

#14 in reply to: ↑ 12 @hlashbrooke
10 years ago

  • Keywords needs-refresh added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to iandunn:

Sorry the for delay Hugh, but this is committed now. Thanks for the patch :)

Let me know how it works out when you use it on your site.

I've tested this out on 2014.capetown and it doesn't seem to be working quite right. You can see the output here: http://2014.capetown.wordcamp.org/speakers/.

The 'Session Speakers' block only includes speakers in the 'talks' tracks and that's working correctly - none of the workshop instructors are appearing there. However, in the 'Workshop Instructors' section it includes everyone - the workshop instructors as well as all the speakers who have already appeared in the first section. There will be some overlap as some speakers should appear in both sections, but right now it is just showing everyone.

I'm going to have a look at the code and see what I can work out, but if this could get sorted out soon that would be awesome - we want to start selling tickets next week :)

#15 @hlashbrooke
10 years ago

  • Keywords needs-refresh removed
  • Resolution set to worksforme
  • Status changed from reopened to closed

OK - it's working now. Looks like it wasn't working purely because I copied and pasted the first shortcode instance and the 'track' parameter was obviously escaped incorrectly. Either way - it's sorted now and the update is working perfectly :)

#16 @iandunn
10 years ago

Glad to hear it's working. Thanks for testing it out :)

This ticket was mentioned in PR #569 on WordPress/wporg-mu-plugins by @dd32.


4 weeks ago
#17

As a followup to #561 some fatal were encountered in a cron task.

It turns out that the flow is something like this:

  1. Cron Load Pattern Directory
  2. This kicks in, realises that it should load Gutenberg-16.8
  3. The Pattern Directory code switches to the Translate.wordpress.org site, and includes the translate plugins
  4. Translate.wordpress.org doesn't have the Gutenberg plugin active
    • active_plugins[ array_search() = false ] translates to active_plugins[0] = gutenberg-16.8/..., which was previously glotpress, not gutenberg.
    • It then loads the other GlotPress plugins that require GlotPress to be active first, and fatals.

Supporting loading plugins for a switched site is not something that WordPress really allows for; but is something that can be done "good enough" if the two sites are running an almost different set of plugins. For example; the Pattern Directory does it like this:
https://github.com/WordPress/pattern-directory/blob/43e0eedfba131cd1fdfc24ac2939a48629acff2e/public_html/wp-content/plugins/pattern-translations/includes/makepot.php#L166-L189

This PR just verifies that the $from plugin is actually in the array before mangling it.

I considered using blog_id checks or checking that $check exists in the array at filter time; but this isn't enough.
Consider the following, SiteA should be filtered to gutenberg-16.8 and if the plugins for SiteB get loaded, it should also load that same version of Gutenberg to avoid conflicts.

  • SiteA: gutenberg, plugin-that-causes-gutenberg-16.8-to-load.
  • SiteB: gutenberg, translation-plugin

@dd32 commented on PR #569:


4 weeks ago
#18

Consider the following, SiteA should be filtered to gutenberg-16.8 and if the plugins for SiteB get loaded, it should also load that same version of Gutenberg to avoid conflicts.

SiteA: gutenberg, plugin-that-causes-gutenberg-16.8-to-load.
SiteB: gutenberg, translation-plugin

This still doesn't "fix" that if the problematic plugin is network activated on SiteA, and per-site activated on SiteB..
To fix that, we could always do the add_filter() for both, if the plugin is detected in either..

The 2nd commit resolves that. Just in case gutenberg ever gets activated on the Translate site (It's network activated, so this code doesn't try to include it at present)

But then to take it a step further, should filter_the_filters be run on switch, to pick up cases like.. loading siteB and then switching to SiteA.. but if the incompatible plugin is already loaded, there's not much we can do...

I think I'm happy with this latest change, as it supports things best we currently can, and should avoid fataling except in the most edge-casey situation.

Note: See TracTickets for help on using tickets.