Skip to content

[FRAME-156]: Add Events#77

Merged
tarecord merged 21 commits into
developfrom
feature/FRAME-156/add-events
Jul 18, 2023
Merged

[FRAME-156]: Add Events#77
tarecord merged 21 commits into
developfrom
feature/FRAME-156/add-events

Conversation

@tarecord
Copy link
Copy Markdown
Member

This adds the ability to trigger specific events with a do_action() so additional data can be collected from sites when users perform specific actions.

@tarecord tarecord self-assigned this Jun 28, 2023
@tarecord tarecord marked this pull request as ready for review June 28, 2023 20:21
Copy link
Copy Markdown
Contributor

@ChrisMKindred ChrisMKindred left a comment

Choose a reason for hiding this comment

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

This all looks good. Should we look into using version/tag branches so you don't have to use the TBD in the since param? I am concerned that merging directly to Main without a version branch could result in that TBD getting left in before a deployment.

Comment thread src/Telemetry/Telemetry/Telemetry.php Outdated
@tarecord
Copy link
Copy Markdown
Member Author

tarecord commented Jun 29, 2023

Yeah, that's a good point and I thought the same thing.

Now that this has been released, should we move to a more strict gitflow workflow:

  • Set up a new develop branch as the default
    • New development is merged directly into this branch
  • Use specific release branches to prep releases
    • Once we decide we're ready for a new release, these are used to prep and merge into main
    • This would be where we update all @since TBD instances to the current release

@ChrisMKindred
Copy link
Copy Markdown
Contributor

I recommend creating a contributing.md file with the details so that it is well documented. GitHub will highlight this file for you when people are making new contributions making it clear what the expectations are.

@tarecord tarecord mentioned this pull request Jul 11, 2023
@tarecord tarecord changed the base branch from main to develop July 11, 2023 16:00
@tarecord
Copy link
Copy Markdown
Member Author

Hey @ChrisMKindred, here's a link to the subset of most recent changes that should make it a little easier to see what I did.

Mainly two things:

  • Moved event-related methods from Telemetry.php to Event.php
  • Updated the Telemetry::send() method to check if the site is currently opted-in before sending

Let me know if you have any questions or suggestions.

@tarecord tarecord requested a review from ChrisMKindred July 14, 2023 20:33
Copy link
Copy Markdown
Contributor

@ChrisMKindred ChrisMKindred left a comment

Choose a reason for hiding this comment

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

Looks good and is fine to merge. Could we add the mocking as a setup function? It is repetitive for us to create the array and function mock in each test.

Comment thread tests/wpunit/EventTest.php
Comment thread tests/wpunit/EventTest.php
@tarecord tarecord merged commit 290f053 into develop Jul 18, 2023
@tarecord tarecord deleted the feature/FRAME-156/add-events branch July 18, 2023 15:23
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.

3 participants