Publishing API with centralized publishing models #41

ormsbee · 2023-04-22T04:13:35Z

chore: Switch from edx-sphinx-theme to sphinx-book-theme
docs: ADR for tagging service. (docs: ADR for tagging service. #40)
feat: add support for static assets
chore: add DRF, remove codecov, rebuild requirements
feat: add publishing API

ormsbee · 2023-04-25T15:25:03Z

Making the barest version of the APIs first (no real error handling). Will flesh out after I have the basic structure of the loading script converted to only make API calls.

ormsbee · 2023-04-26T04:48:31Z

@bradenmacdonald, @kdmccormick, @feanil: If you have time, could you please take a look at the model.py files, particularly for the publishing and components apps (the contents app just has RawContent and TextContent extracted out of components without real modifications).

The API layer is a mess right now, and probably not worth looking at. I wanted to get it to a point where load_components.py could do all its work without manipulating the models directly. And that does work, but there's no proper error handling and the function signatures are inconsistent. I probably need another day of hacking at it to get that part into a reviewable state, and I need to step away from this for a few days. But I could use early feedback on whether the data model looks reasonable.

Thanks folks.

kdmccormick

I think the data model and how you've split it between the three apps looks great. All I have is a question about LearningPackage and some docstring nitpicks.

kdmccormick · 2023-04-26T20:03:55Z

openedx_learning/core/components/api.py

+        component_version=component_version,
+        raw_content_id=raw_content_id,
+        identifier=identifier,
+        learner_downloadable=False,


Suggested change

learner_downloadable=False,

learner_downloadable=learner_downloadable,

I know you said not to look at api.py; just wanted to point this out so that it doesn't cause you headache down the road.

kdmccormick · 2023-04-26T20:09:06Z

openedx_learning/core/publishing/models.py

+    If you are referencing this model from within the same process, use a
+    foreign key to the id. If you are referencing this PublishedEntity from an
+    external system/service, use the UUID. The identifier is the part that is
+    most likely to be human-readable, and may be exported/copied, but try not to
+    rely on it, since this value may change.


Very helpful guidelines 👍🏻

openedx_learning/core/publishing/models.py

kdmccormick · 2023-04-26T20:46:39Z

openedx_learning/core/publishing/models.py

+    It would have saved a little space to add this data to the Published model
+    (and possibly call the combined model something else). Split Modulestore did
+    this with its active_versions table. I keep it separate here to get a better
+    separation of lifecycle events: i.e. this table *only* changes when drafts
+    are updated, not when publishing happens. The Published model only changes
+    when something is published.


Thanks for this context. I like the separation too.

kdmccormick · 2023-04-26T20:56:51Z

openedx_learning/core/publishing/models.py

 class LearningPackage(models.Model):
+    """
+    Top level container for a grouping of authored content.
+
+    Each PublishableEntity belongs to exactly one LearningPackage.
+    """
+


Now that entities are all individually publishable, what role does LearningPackage play in the publishing app?

I had the same question. I guess it's about namespacing identifiers?

I have the vague impression that the publishing app is not as generic as it could be when it's tied to the concept of LearningPackage. But maybe I'm just not clear on the role of LearningPackage.

Providing a namespace for identifiers is a big one. The other thing is that I wanted some sort of wrapping data structure that was roughly equivalent to a course or library, since this is how a lot of content will be organized.

kdmccormick · 2023-04-26T20:57:21Z

openedx_learning/core/contents/models.py

+    The reason this is built directly into the Learning Core data model is
+    because we want to be able to easily access and browse this data even if the
+    app-extended models get deleted (e.g. if they are deprecated and removed).


This was helpful for me 👍🏻

kdmccormick · 2023-04-26T21:02:14Z

.importlinter

    openedx_learning.core.components
+    openedx_learning.core.contents
    openedx_learning.core.publishing


I love this--it conveys so much about the relationship between the apps in one glance.

kdmccormick

OK, I thought of real critique:

Django has built-in support for multi-table inheritance: https://docs.djangoproject.com/en/3.2/topics/db/models/#multi-table-inheritance

What do you think about using that instead of the the PublishableEntity{Version} / PublishableEntity{Version}Mixin combo? I think the resulting data model would be identical, but it'd be more idiomatic Django.

ormsbee · 2023-04-27T01:30:00Z

@kdmccormick: That's a good point. I think I was turned off of multi-table inheritance years ago because of how strongly certain books (like Two Scoops) advise against it. My understanding was that it did a lot of surprising left outer joins. But that was years and years ago, I haven't really tested it, and you're right in that it looks like a good fit as long as those joins aren't forced onto the base class (i.e. the publishing app can still do its thing without ever joining on sub-class tables).

ormsbee · 2023-04-27T01:53:14Z

@kdmccormick: Thinking on it a bit more, the reverse relations also become a bit weirder. For instance, in a non-inheritance model, the versions relation from PublishableEntity refers to PublishableEntityVersion, and Component has its own versions relation set to ComponentVersion. But if Component inherits from PublishableEntity, then component.versions would map to PublishableEntityVersion.

kdmccormick · 2023-04-27T16:45:14Z

@ormsbee If that's the main issue, then we could avoid it by calling the generic reverse relation PublishableEntity.entity_versions, which would allow the subclasses define .versions. So you'd have:

component.versions              # ComponentVersions
component.publishableentity_ptr # PublishableEntity
component.entity_versions       # PublishableEntityVersions

I think I was turned off of multi-table inheritance years ago because of how strongly certain books (like Two Scoops) advise against it. My understanding was that it did a lot of surprising left outer joins. But that was years and years ago, I haven't really tested it, and you're right in that it looks like a good fit as long as those joins aren't forced onto the base class

That's good to know; I think I've only ever used it once, and it wasn't in a situation where performance was top-of-mind. If you think using Django's inhertance would lead to more surprising queries then I am OK with the hand-rolled version you have now, which at the very least is easy for us to reason about.

bradenmacdonald

Makes sense overall, though my vague gut feeling is that the publishing app seems to own too much logic about how things are grouped (LearningPackage) and identified (id, uuid, and identifier are part of PublishableEntity).

For example, does the publishing app even use the UUID field? If not, why isn't it up to each publishable model like Component to decide if it wants a UUID or not? I feel like it would be nice if the Publishable models/mixins could be configured to use any sort of namespacing/identifier scheme that the Publishables themselves want to use. But perhaps that adds too much complexity or abstraction.

Edit: after a night's sleep, I think this does make sense to me. Published things need to have the right identifiers so they can be used throughout the system, and the UUID is serving the role of ISBN in physical book publishing.

bradenmacdonald · 2023-04-28T01:57:12Z

openedx_learning/core/components/models.py

+    # query by other Component fields within a given LearningPackage, which is
+    # going to be a common use case (and we can't make a compound index using
+    # columns from different tables).
    learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE)


We could add a constraint like models.CheckConstraint(check=Q(publishable_entity__learning_package=F("learning_package")), name='learning_package') to ensure this is always the same at the database level.

Added this, thank you!

Actually, nvm, this breaks:

❯ python manage.py migrate SystemCheckError: System check identified some issues: ERRORS: oel_components.Component: (models.E041) 'constraints' refers to the joined field 'publishable_entity__learning_package'.

openedx_learning/core/publishing/models.py

bradenmacdonald · 2023-04-28T02:10:00Z

openedx_learning/core/publishing/models.py

+    ``uuid`` should be treated as immutable.
+
+    PublishableEntityVersions are created once and never updated. So for
+    instance, the ``title`` should never be modified.


So if I make 100 small edits (to a draft) without publishing, there will still be 100 PublishableEntityVersion rows created, but just one Draft row which gets updated each time, and no PublishLog[Record]/Published rows. Is that right?

bradenmacdonald · 2023-04-28T02:15:32Z

openedx_learning/core/publishing/models.py

 class LearningPackage(models.Model):
+    """
+    Top level container for a grouping of authored content.
+
+    Each PublishableEntity belongs to exactly one LearningPackage.
+    """
+


I had the same question. I guess it's about namespacing identifiers?

I have the vague impression that the publishing app is not as generic as it could be when it's tied to the concept of LearningPackage. But maybe I'm just not clear on the role of LearningPackage.

ormsbee · 2023-04-28T14:43:36Z

@kdmccormick:

@ormsbee If that's the main issue, then we could avoid it by calling the generic reverse relation PublishableEntity.entity_versions, which would allow the subclasses define .versions. So you'd have:
component.versions              # ComponentVersions
component.publishableentity_ptr # PublishableEntity
component.entity_versions       # PublishableEntityVersions

Right, I realize we can rename things to avoid the collision in this case. I meant the general notion that inheritance will conflate relations between things in the publishing app and things in the apps that build on it since we'll be dragging everything into the same namespace, and I'm not sure:

Is it worth it for the ergonomics/convenience?
Is it worth making the field names clunkier on the publishing side?
Will people shoot themselves in the foot with this somehow, because we'll add a relation to a publishing model at some point that breaks a subclass?
Is subclassing better than making a mixin in terms of improving ergonomics?

I feel like the most intuitive thing for developers would be to have something where they can say:

component.published_version  # This should be a ComponentVersion
component.draft_version  # This should be a ComponentVersion

So something where the part of the API that content apps are exposed to runs through publishing's models but spits back their own models, so they're not actually exposed to any of the publishing details. But that means making some little helper method (either inherited or a mixin) either way. Given that, I'm leaning more towards clunky-but-less-coupled (i.e. no inheritance). But I'll experiment with it some.

kdmccormick · 2023-04-28T15:19:44Z

@ormsbee Thanks for considering it. When it comes to the question:

Is it worth it for the ergonomics/convenience?

you've been actually working in this codebase and I haven't, so I think you have a much better intuition for what will feel
good for developers using this API :)

I feel like the most intuitive thing for developers would be to have something where they can say:
 component.published_version  # This should be a ComponentVersion
 component.draft_version  # This should be a ComponentVersion

I agree, that does look very intuitive.

It would be a change to your data model, but I think using generic relations you could achieve something like this? Some pseudo-Django:

class VersionRegistry(Model):
    """
    Links versionless things to their draft and published versions.
    """
    # The pair of concrete types. Component/ComponentVersion, Unit/UnitVersion, etc.
    verionless_type = ForeignKey(ContentType)
    versioned_type = ForeignKey(ContentType)

    # The concrete versionless entity. An instance of a Component, Unit, etc.
    versionless_id = PositiveIntegerField()
    versionless = GenericForeignKey('versionless_type', 'versionless_id', related_name="versioning")

    # The concrete published and draft entity versions. An instance of ComponentVersion, UnitVersion, etc.
    published_id = PositiveIntegerField()
    published = GenericForeignKey('versioned_type', 'published_id', related_name="versioning")
    draft_id = PositiveIntegerField()
    draft = GenericForeignKey('versioned_type', 'draft_id', related_name="versioning")

For developers using components, it'd look like:

component.versioning.published  # a ComponentVersion
component.versioning.draft      # a ComponentVersion

# or, starting with a draft component:
draft.versioning.published # a ComponentVersion
draft.versioning.versionless  # a Component, equivalent to `draft.component`

(disclaimer: I have no idea what the queries patterns would look like)

(for an Open edX example of this, credentials uses a generic relation in order to tie users to the multiple different types of credentials in one table).

ormsbee · 2023-04-28T15:45:38Z

Makes sense overall, though my vague gut feeling is that the publishing app seems to own too much logic about how things are grouped (LearningPackage) and identified (id, uuid, and identifier are part of PublishableEntity).

For example, does the publishing app even use the UUID field? If not, why isn't it up to each publishable model like Component to decide if it wants a UUID or not? I feel like it would be nice if the Publishable models/mixins could be configured to use any sort of namespacing/identifier scheme that the Publishables themselves want to use. But perhaps that adds too much complexity or abstraction.

As an aside, publishing would use the UUID directly if there was a proper REST API hooked up to it.

FWIW, this was something I struggled with in the publishing/components split, especially since Component already has its own tuple of (namespace, type, local_identifier). At the end of the day, I feel like this is a place where we've gotten tripped up enough in the past that I wanted to take a strong baseline position for all publishable things–it will have at least these three id-related things, and you're free to add more to your own models if necessary. We have other services, and those services will want to store references to content and versions of content, and having the UUID means that they can do so in a way that still gives us some chance of changing CourseKeys and the like some day.

I also did want a LearningPackage-wide identifier that was app-chosen and locally unique, so that there was a common way to look things up within a LearningPackage.

Honestly, the place where I feel that I'm on the shakiest ground is UUIDs for Versions of things because with the way versions work right now, every version is uniquely identifiable by a combination of PublishableEntity UUID + version_num. I have a possibly irrational dislike of compound-ids, and I wasn't sure about the version_num thing in the beginning, though I'm more committed to it now. I'm thinking of taking out the UUIDs on EntityVersion largely because I'm not sure how we'd handle them properly in import/export. (At least with Entities, we could export a mapping file of UUIDs to identifiers.)

Edit: I guess versioning doesn't explicitly make it into the plain OLX import/export either way. Still thinking of killing the UUID on versioned things...

ormsbee · 2023-04-28T15:51:07Z

Edit: after a night's sleep, I think this does make sense to me. Published things need to have the right identifiers so they can be used throughout the system, and the UUID is serving the role of ISBN in physical book publishing.

:-) I somehow missed this part. You said it much more succinctly than I did. I'm going to steal this analogy for the docstring.

ormsbee · 2023-05-09T02:01:12Z

I don't know why this only occurs to me now, but a Grading Policy would be a totally reasonable thing to model as a PublishableEntity.

ormsbee · 2023-05-09T02:02:00Z

Which also leads me to weird places. Would we ever have a library of grading policies and taxonomies? 😛

ormsbee · 2023-05-09T17:41:39Z

tests/openedx_learning/core/components/test_models.py

+        assert component.versioning.draft == component_version
+        assert component.versioning.published is None
+        publish_all_drafts(self.learning_package.pk, published_at=self.now)
+
+        # Force the re-fetch from the database
+        component = get_component_by_pk(component.pk)
+        assert component.versioning.published == component_version
+
+        # Grabbing the list of versions for this component
+        assert list(component.versioning.versions) == [component_version]


@kdmccormick: the versions helper interface that you suggested (though it's implemented by hand through existing model relations instead of via generic foreign keys).

ormsbee · 2023-05-09T18:01:59Z

@kdmccormick, @bradenmacdonald, @feanil: Made updates to the models.

@kdmccormick: I ended up going for the non-inheritance approach, with a helper in the mixin. I tried it with generic relations and it's workable, but I felt like it added fields that weren't really necessary and I worried that it would encourage a pattern where publishing starts to know more about the specific things being published.

I'm going to fix up the APIs more, but I think I'd like to just land them in a rough shape. They'll probably need a few followup PRs to refine them, but I want to land the data models in a solid enough shape where we can start putting actual data into them (i.e stop my "blow away the world and remake the migrations" approach), and I think this PR will achieve that. The API modules at this point are mostly just to have a convenient proof-of-concept to dump stuff in and out of those models.

feanil

I looked just at the publishing models and they make sense to me. I had some questions/clarification requests for docstrings but nothing blocking.

feanil · 2023-05-10T13:36:20Z

openedx_learning/core/publishing/model_mixins.py

+
+            # This will FAIL because it's going to use the relation value
+            # cached on component instead of going to the database again.
+            # You need to re-fetch the component for this to work.


How would you do the re-fetching, by running refresh_from_db or something else? Any chance you can document what the correct thing to do here as well?

I'll add it, but basically you have to refetch it from the database. I'm hesitant to build anything smart around that because that's more of a Django ORM thing.

@ormsbee Did you consider the collapsing VersioningHelper directly into the PublishableEntityMixin class?

In addition to simplifying the class structure, having component.published instead of component.versioning.published might be more immediately obvious to devs that .published will not automatically be updated without a re-fetch.

If I pretend I don't know this codebase, the .versioning. attribute in the middle kinda hints "magic happens here", even though the it's really just a syntactic helper construct.

@ormsbee Did you consider the collapsing VersioningHelper directly into the PublishableEntityMixin class?

I did. In fact, my original comment was proposing that. But I liked your suggestion of putting it behind versioning better, because I expect that a bunch of other stuff will make it into this class over time.

In addition to simplifying the class structure, having component.published instead of component.versioning.published might be more immediately obvious to devs that .published will not automatically be updated without a re-fetch.

If I pretend I don't know this codebase, the .versioning. attribute in the middle kinda hints "magic happens here", even though the it's really just a syntactic helper construct.

I mean, it is a little magical, right? At least when compared to the simpler properties that just proxy to fields in the accompanying publisher app models (like uuid). But it's relatively small magic.

Maybe I should just change my implementation to always refetch. That's probably the more intuitive thing anyway, right?

Gotcha. Sorry for the suggestion whiplash. When I suggested the .versioning. interface, I was thinking of it as an alternative to PublishableEntity{Version}Mixin. Since we're sticking with the mixins, I could go either way on .versioning.

I think:

component.published implies no re-fetch.

component.versioning.published could go either way.

component.get_versioning().published would heavily imply re-fetch.

I'm going to stick with versioning for now. It is a bit magical, but I think it's more consistent with Django ORM conventions.

feanil · 2023-05-10T14:17:43Z

openedx_learning/core/publishing/models.py

+    Note: When we actually implement the ability to change identifiers, we
+    should make a history table and a modified attribute on this model.
+
+    Why are Identifiers in this Model?


It's nice to have these here but they also feel like ADRs. Maybe it makes sense to extract them eventually?

Fair point. Making a short one around identifiers now. I'll punt others until later.

feanil · 2023-05-10T15:00:53Z

openedx_learning/core/publishing/models.py

+    1. No Draft entry for a PublishableEntity: This means a PublishableEntity
+       was created, but no PublishableEntityVersion was ever made for it, so
+       there was never a Draft version.
+    2. A Draft entry exists and points to a PublishableEntityVersion: This is


Is this the state when the PublishableEntityVersion is also the latest published one?

Not necessarily. The Draft entry will always point to the most recently created PublishableEntityVersion. That may or may not be the published version–that depends on the entry in the Published model.

ormsbee · 2023-05-10T19:59:09Z

openedx_learning/core/publishing/apps.py

    name = "openedx_learning.core.publishing"
    verbose_name = "Learning Core: Publishing"
    default_auto_field = "django.db.models.BigAutoField"
+    label = "oel_publishing"


Flagging this for reviewers: does this sort of prefix label sound okay? I'm a little worried about namespace collision with app names in the LMS and potentially elsewhere (since the Django app namespace is flat), and I thought having a repo-wide prefix would help. It does mean that it's a little less intuitive when you do things like makemigrations, since the app will now be oel_publishing instead of publishing.

Looks reasonable to me.

You could be super explicit and do openedx_learning_publishing. If that feels too verbose, though, then oel_publishing is good.

kdmccormick

Looking good, just a couple questions.

FYI, I paid closer attention to the models and model_mixins than I did the apis or load_components.

openedx_learning/core/publishing/models.py

kdmccormick · 2023-05-11T16:52:48Z

openedx_learning/core/publishing/api.py

+    There may be a more clever way to introspect this information from the model
+    metadata, but this is simple and explicit.
+    """
+    return PublishableContentModelRegistry.register(


Consider doing an isinstance check first to make sure that the arguments are sublclasses of PublishableEntityMixin and PublishableEntityVersionMixin, respectively. I feel like the error would be very hard to track down if, say, the arguments were swapped by mistake.

Added issubclass checks. Now it'll fail startup if it's improperly configured, with an error like:

raise ImproperlyConfigured( django.core.exceptions.ImproperlyConfigured: <class 'openedx_learning.core.components.models.ComponentVersion'> must inherit from PublishableEntityMixin

kdmccormick · 2023-05-11T17:06:48Z

openedx_learning/core/publishing/model_mixins.py

+
+            # This will FAIL because it's going to use the relation value
+            # cached on component instead of going to the database again.
+            # You need to re-fetch the component for this to work.


Gotcha. Sorry for the suggestion whiplash. When I suggested the .versioning. interface, I was thinking of it as an alternative to PublishableEntity{Version}Mixin. Since we're sticking with the mixins, I could go either way on .versioning.

I think:

component.published implies no re-fetch.

component.versioning.published could go either way.

component.get_versioning().published would heavily imply re-fetch.

openedx_learning/core/publishing/model_mixins.py

kdmccormick · 2023-05-11T17:15:21Z

openedx_learning/core/publishing/api.py

+    return package
+
+
+def create_publishable_entity(learning_package_id, key, created, created_by):


As a general comment: The class hierarchy in openedx-learning is becoming non-trivial. That's not suprising, since we're tackling a complex problem, but having type annotations on all of these API functions could make it much easier to grok. I know you're still iterating on the APIs, so don't consider this blocking.

Agreed, all api.py functions should have annotations.

I'll add those annotations when I add the action that actually checks them in CI. 😞 I need to do some care and feeding for the CI in this next week.

kdmccormick · 2023-05-11T18:09:14Z

openedx_learning/core/publishing/api.py

+        for draft in draft_qset.select_related("entity__published"):
+            # There might not be a currently published version for this
+            # PublishableEntity yet.
+            if hasattr(draft.entity, "published"):


@ormsbee sorry, I'm confused about this too: how does published get assigned on the PublishableEntity instance? Why is it not always available on the instance? I would have expected this check to look like:

if draft.entity.published:

The Published model a OneToOneField pointing to PublishableEntity, so you can access the relationship in both directions. But when you're coming through the reverse relation (entity.published), there's a chance that there is no corresponding row in Published. When that happens, Django will throw a ObjectDoesNotExist if you try to access it, and they recommend checking with hasattr:

https://docs.djangoproject.com/en/3.2/topics/db/examples/one_to_one/

Ah, I thought it would be something like that, but I was expecting the ObjectDoesNotExist try-except. When I saw hasattr I assumed it must be something more complicated.

I would prefer the try-except here; Django's hasattr recommendation here strikes me as very non-explicit and unpythonic.

ormsbee · 2023-05-11T21:23:24Z

@kdmccormick: I think I've addressed all your comments and suggestions.

This is a major rethinking of the core data apps: * Parts of the components app were extracted into publishing and a new contents app. * The publishing app models are now entirely responsible for the draft and publish state management. * There is now a startup registration step for content models that want to build off of publishing's PublishableEntity and PublishableEntityVersion (register_content_models). See the ComponentsConfig class for an example of it in use. * Many model and field names were changed to make their intent clearer. (See comments in the models and ADRs in this commit for details.) * Apps now carry an "oel_" prefix in their labels. * An interim API layer was created, though this is still very much a work in progress, and should not be considered stable.

kdmccormick

🚀🚀🚀

Enabling WAL on dev.db will generate extra dev.db-shm and dev.db-wal files that we want git to ignore..

kdmccormick reviewed Apr 26, 2023

View reviewed changes

bradenmacdonald reviewed Apr 28, 2023

View reviewed changes

ormsbee mentioned this pull request Apr 30, 2023

Pattern for *PublishLogEntry to enable easy reverts #37

Closed

ormsbee commented May 9, 2023

View reviewed changes

feanil approved these changes May 10, 2023

View reviewed changes

ormsbee changed the title ~~(WIP) Publishing API with centralized publishing models~~ Publishing API with centralized publishing models May 10, 2023

ormsbee marked this pull request as ready for review May 10, 2023 16:05

ormsbee commented May 10, 2023

View reviewed changes

kdmccormick reviewed May 11, 2023

View reviewed changes

kdmccormick approved these changes May 11, 2023

View reviewed changes

ormsbee added 2 commits May 11, 2023 21:36

feat: add ADRs around identifier conventions, app labels

660ae88

feat: ignore dev.db-related files

c4fd319

Enabling WAL on dev.db will generate extra dev.db-shm and dev.db-wal files that we want git to ignore..

ormsbee force-pushed the centralize-publishing-models-3 branch from cdd836f to c4fd319 Compare May 12, 2023 01:36

ormsbee merged commit c0d36d9 into openedx:main May 12, 2023

ormsbee deleted the centralize-publishing-models-3 branch May 12, 2023 01:37

ormsbee mentioned this pull request May 12, 2023

Discovery: Determine how v2 Content Libraries can make use of Learning Core #30

Closed

ormsbee self-assigned this May 12, 2023

	learner_downloadable=False,
	learner_downloadable=learner_downloadable,

		return package


		def create_publishable_entity(learning_package_id, key, created, created_by):

Publishing API with centralized publishing models #41

Publishing API with centralized publishing models #41

Uh oh!

Conversation

ormsbee commented Apr 22, 2023

Uh oh!

ormsbee commented Apr 25, 2023

Uh oh!

ormsbee commented Apr 26, 2023

Uh oh!

kdmccormick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdmccormick left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormsbee commented Apr 27, 2023

Uh oh!

ormsbee commented Apr 27, 2023

Uh oh!

kdmccormick commented Apr 27, 2023

Uh oh!

bradenmacdonald left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormsbee commented Apr 28, 2023

Uh oh!

kdmccormick commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ormsbee commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ormsbee commented Apr 28, 2023

Uh oh!

ormsbee commented May 9, 2023

Uh oh!

ormsbee commented May 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormsbee commented May 9, 2023

Uh oh!

feanil left a comment

Choose a reason for hiding this comment

kdmccormick left a comment •

edited

Loading

bradenmacdonald left a comment •

edited

Loading

kdmccormick commented Apr 28, 2023 •

edited

Loading

ormsbee commented Apr 28, 2023 •

edited

Loading