Proposal: Event repository

[psrpinto]

Please read this description before reviewing the PR :) It provides important context that I think is essential to consider while reviewing.

I would recommend reviewing the final result instead of individual commits.

Recently, we identified the need to refactor the code that retrieves active events, as documented in #117. While looking into that issue, I realised that there was opportunity for a more extensive refactor, that would address #117 but also improve other parts of the codebase.

I took the liberty to implement such a refactor, and I'm proposing it in this PR. If you agree that this proposal is beneficial to the project, we can iterate on it here until we're happy with the result. If you don't agree, we can scrap this PR.

Note that the code introduced in here is not being called anywhere. Once/if this PR is merged, I will make it so that the codebase uses the code introduced here.

As always, I welcome any feedback you might have.

The problem

Before going into the proposed solution, I think it's important to provide context on what problems this PR solves. What follows are some examples I identified that are addressed, or at least improved by this PR.

Duplication of code

An example of this is retrieving active events (#117), where we have the same logic with slight variations in multiple places:

1. When storing stats
2. When displaying the active events of a user
3. When displaying the currently active events in the "homepage" (/events)

Duplication of validation and/or incomplete validation

An example of this is creating an instance of an event. Whenever we need to create an instance of an event, we need to perform the same checks on the event. For example, checking that the $end is after the $start, or that the $title is not empty.

There are also instances where we are not fully validating the event data, for example, making sure that the title is not empty.

Duplication of other checks

An example of this is validating that the post_type is event. This needs to be done every time an event is retrieved from the database. It should not be possible for the developer to retrieve an event from the database that is not of type event.

Side-effects not guaranteed

An example of this is making sure that the active events cache is invalidated when events are created of modified. The developer must remember to invalidate the cache when required.

Data access is hard to cover by unit tests

Because data access is spread throughout the codebase, it's difficult to cover with unit tests. We would only be able to test data access indirectly (by testing other features), but ensuring the validity of the stored data is essential, so it should be relatively easy for the developer to cover data access with tests.

Error prone

IMHO, the combination of the above issues, results in error-prone code, because it's easy for the developer to make mistakes, because they need to consider a large number of things when making changes.

For example, when a developer is writing business logic (currently in the Route class and main plugin file), I think they should not need to consider details of data access like validating that the post has type event.

IMHO all the little things the developer must consider add up, and make for a codebase that is hard to understand and maintain.

Proposed solution

To address some of the issues mentioned above, this PR introduces a Data access layer for events. This layer can be thought of as a black box that abstracts details of how events are stored, through a higher-level API for manipulating and retrieving events.

This API is specified by the Event_Repository_Interface, which I believe contains all the operations we currently need to perform on events:

interface Event_Repository_Interface {  
    public function create_event( Event $event ): void;  
    public function update_event( Event $event ): void;  
    public function get_event( int $id ): Event;  
    public function get_current_events( int $page = -1, int $page_size = -1 ): Events_Query_Result;  
    public function get_current_events_for_user( int $user_id, int $page = -1, int $page_size = -1 ): Events_Query_Result;  
    public function get_past_events_for_user( int $user_id, int $page = -1, int $page_size = -1 ): Events_Query_Result;  
    public function get_events_created_by_user( int $user_id, int $page = -1, int $page_size = -1 ): Events_Query_Result;  
}

Event represents an event, and contains all the data of the event, e.g. $start, $end, $timezone, $title, etc. It is not possible for there to be an instance of Event that contains invalid data. The Event constructor and setters perform validation, which ensures we're validating as soon as possible, before data gets stored in the database.

Operations that retrieve a list of events (e.g. get_current_events() or get_events_created_by_user()) return an instance of Events_Query_Result, which carries the retrieved events themselves, and information needed for pagination (e.g. total number of results).

There are two implementations of Event_Repository_Interface:

1. Event_Repository: this is the "normal" repository
2. Event_Repository_Cached: extends Event_Repository and overrides methods that require caching.

The reasoning for having an interface and two different implementations is so that calling code does not depend on a specific implementation. The calling code should not need to care whether caching is enabled or not.

Finally, this "black box" is covered by some unit tests, so we can be reasonably confident that it will operate as expected.

[psrpinto]

This will be removed in upcoming PRs, if/when this one is merged.

[akirk]

@psrpinto the tests are failing, can you take a look?

[psrpinto]

Tests are now passing, it was due to the change of the post type from event to translation_event.

[amieiro]

Could we use the same approach that we use in with the posts: insert_event( Event $event ) to create and update the event and update_event( Event $event ) to update the event.

I think it will be interesting that both methods will return int|WP_Error, where int is the event ID.

And I think we need a method to delete the events: delete_event( int $eventid, bool $force_delete = false ): Event|false:

• Parameters:
  • $eventid: the id of the event to delete.
  • $force_delete: delete the event if it has translations. false by default.
• Returned value: Event|false Event data on success, false on failure.

wp_insert_post() and wp_update_post() documentation.

[psrpinto]

I have now implemented these suggestions.

Concerning delete_event(), for now the function doesn't take the $force_delete argument. I didn't do it here because I'm not sure it's something that belongs in the repository as it feels like it's more a business logic thing, so it would be the calling code that needs to check if the event has stats before calling delete_event(). That's already the case today, here.

Currently (in trunk) we're not yet deleting stats when the event is deleted, and I would prefer to not add any features in this PR, so that would be another reason to not add the $force_delete flag yet.

If this makes sense to you, I can create a new issue for deleting stats when the event is deleted.

[psrpinto]

Great feedback @amieiro, thank you! I will implement your suggested changes and fix conflicts with trunk.

[amieiro]

If you execute the PHPCS with the WordPress standard for this file

phpcs includes/event/event-repository-interface.php --standard=WordPress

you will see some errors, like each class should be in its own file. I know it is some extreme to split these small classes in different files, but this is what the WPCS suggests. You can see here the PR for this:

Best practice suggestion: Declare only one class/interface/trait in a file.

[psrpinto]

There are quite a few rules that we are disabling. I would prefer to not make changes to phpcs in this PR, as that will require changing code that is not introduced by this PR, and in this PR the intention was just to add the repository without making any changes to the existing code.

If we want to change the phpcs rules, my suggestion would be to do that in a PR that only does that, as it will likely result in many changes across the codebase.

[psrpinto]

I've now changed get_event() so that it returns null instead of throwing an EventNotFound exception, as returning null is more in line with how get_post() behaves.

[akirk]

Let's go ahead with it, thank you! If we find more we can always iterate some more.

[amieiro]

As this function is specific for this class (it is not implementing a method from the interface), it will be interesting to add the description to this method and to the other methods that are specific for only the current class.