Skip to content

Add custom post type plugins for News site #74

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Mar 23, 2023
Merged

Add custom post type plugins for News site #74

merged 5 commits into from
Mar 23, 2023

Conversation

matt-bernhardt
Copy link
Member

@matt-bernhardt matt-bernhardt commented Mar 17, 2023 •
edited by JPrevost
Loading

This PR now does a few things:

  1. Creates four new plugins for the application:
    • One each for the custom content types used by the News site (Bibliotech articles and Spotlights)
    • One each for site-specific extensions to the built-in Post and Page content types
  2. Refactors the MITlib Post must-use plugin which is at the heart of our content modeling, to allow for smarter storage of the defined fields and field groups among all the descendant plugins. Rather than store information in a common directory (which is loaded multiple times and results in unintended fields being created), the plugin now separates its storage, with a subfolder for each descendant plugin.
  3. Existing field groups which have already been created are split into these newly-created folders.
  4. For field groups (in the News site, imported to this application during this PR) which add the same fields to multiple content types, the file is split among each subfolder. This maintains a separation of concerns, so for example only fields added to the Bibliotech content type are stored in the /web/app/mu-plugins/mitlib-base/data/bibliotech, and no fields are added from any other location. This comes at the cost of a bit of duplication, so if we need to change a field that is common to multiple content types, multiple changes will be needed.

Something that I'm grappling with, and trying not to fall down the rabbit hole of fixing All The Debt, is the discovery that we've been inconsistent in how these are registered. Within WordPress, there is a mix of singular and plural words for the content type:

To this, we are adding:

  • bibliotech
  • spotlights

...which will make four plural names, and two singular names. These are the names in use on the legacy site - changing them here, although tempting, feels like a change that could require a pretty large changeset.

Within the classes that define these content types, and thus the folders that store their field definitions, we are consistently using singular names (so the class name for Spotlight records is Mitlib\PostTypes\Spotlight, the storage folder is ../data/spotlight/, but the URL for creating a new record is /wp-admin/post-new.php?post_type=spotlights and the template which displays a spotlight would be something like page-spotlights.php.

Key question for code review

  1. Do you agree with the choice to preserve the existing post type names, rather than standardizing on either the singular or plural names?

Developer

Secrets

  • No secrets are affected

Documentation

Accessibility

  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)

Stakeholder approval

  • Stakeholder approval is not needed yet

Dependencies

NO dependencies are updated (plugins are created in this repo, but we don't change external dependencies)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • The changes have been verified
  • The documentation has been updated or is unnecessary
  • New dependencies are appropriate or there were no changes

@matt-bernhardt
Add basic Bibliotech post type
8170a4c 
** Why are these changes being introduced:

* We need to create a post type for Bibliotech articles

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/lm-149

** How does this address that need:

* This creates a "Mitlib Post Bibliotechs" plugin, which defines the
  shell of the Bibliotech post type.
* Please note that this is only the post type itself, and the set of
  relevant fields will be created under LM-150.

** Document any side effects to this change:

* None (subject to the above bullet points about the scope of this work)
@matt-bernhardt
Add basic Spotlight post type
766ae90 
** Why are these changes being introduced:

* We need to create a post type for Spotlight articles

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/lm-149

** How does this address that need:

* This creates a "Mitlib Post Spotlights" plugin, which defines the
  shell of the Spotlight post type.
* Please note that this is only the post type itself, and the set of
  relevant fields will be created under LM-150.

** Document any side effects to this change:

* None
@matt-bernhardt matt-bernhardt requested a review from JPrevost March 20, 2023 21:05
@matt-bernhardt
Copy-pasted news site field groups
6a7a650 
** Why are these changes being introduced:

* There are 14 different field groups defined within the News site,
  which need to be brought over to the new application.

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/lm-150

** How does this address that need:

* This exports the 14 legacy field groups to the mitlib-post/data
  folder, where our other field groups are defined. Each file is named
  using the key for that group.
* No changes are made to these files in this commit - that will be next.

** Document any side effects to this change:

* None, although 14 files is a code smell.
@matt-bernhardt
Update news site field group definitions
77841c7 
** Why are these changes being introduced:

* The exported field groups added in the last commit need updates to be
  loaded correctly, and need better documentation

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/lm-150

** How does this address that need:

* This updates the field group JSON files to allow them to be loaded
  correctly. It also updates the group titles and descriptions, which
  are seen in the field groups admin screen (but does not change either
  the field names themselves, or how they appear on the record edit
  screen).

** Document any side effects to this change:

* I need to investigate whether these are properly scoped for this
  approach; I worry that this approach will leak these fields to other
  sites that use Post records (which is about half the site).
@matt-bernhardt
Refactor ACF field storage and MITlib Post plugin
acb5bef 
** Why are these changes being introduced:

* We realized that the current "all fields stored in the same folder"
  approach to storing fields and field groups produced a bug, in which
  unwanted fields would be implemented in certain circumstances (such as
  when multiple posts or sites extended the same built-in post types in
  different ways, or when a site wanted to implement only one post type
  but leave the built-in types unaffected).

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/lm-150

** How does this address that need:

* This refactors the load_point and save_point methods in the MITLib
  Post plugin, so that when called those methods first determine which
  descendant class has called the method. This is used to load only a
  specific sub-directory of the data folder, preventing field leakage
  that was not intended by site builders.
* This also creates two additional site-specific plugins for extending
  build-in post types, for the News site and Parent site specifically.
  Dealing with the Parent site is needed now because the previous bullet
  point meant that some existing field groups needed a home, or they
  would disappear during this work. Creating this site-specific plugin
  gives those fields a home.
* The prior approach to grouping field definitions by function, so that
  a single JSON file would add a batch of fields to multiple post types,
  has been abandoned. Those JSON files which did this have been split
  apart, so that - for example, the file group_54dd062c84904.json in
  the data folder is now group_54dd062c84904-1.json under the sitenews
  folder (for adding the flag to the Post record), group_54dd062c84904-2
  in the bibliotech folder (for adding the flag to the Bibliotech
  record), and group_54dd062c84904-3.json under the spotlight folder
  (for adding the flag to the Spotlight post type).

** Document any side effects to this change:

* Some existing field leakage which is already visible will now be
  resolved. For an example, note that in the Live tier right now,
  if you create a Post record under the Exhibits site, you will have an
  option to make that record an Alert - which should not be the case.
JPrevost
JPrevost approved these changes Mar 23, 2023
View reviewed changes
Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

I was able to confirm custom values when create a News post locally with this branch. I didn't dig deep into the various other changes as that felt better to let UX review as they'll better understand if everything works as expected.

Nice solution to fix the bug with the various sites accidentally loading config from all the sites.

@matt-bernhardt matt-bernhardt merged commit 3f156b8 into master Mar 23, 2023
@matt-bernhardt matt-bernhardt deleted the lm149 branch March 23, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JPrevost JPrevost JPrevost approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@matt-bernhardt @JPrevost