Skip to content
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.

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

Add correct handled/unhandled state to notifications #325

Merged
merged 44 commits into from
Nov 1, 2018

Conversation

Cawllec
Copy link
Contributor

@Cawllec Cawllec commented Oct 26, 2018

Goal

Adds the correct handled/unhandled state to notifications
Adds end-to-end tests to check the added functionality

Changeset

Added

  • Adds UnhandledState middleware to determine whether handled or not an event is handled

Tests

Added unhandled and handled maze-tests for end to end testing of unhandled feature
TODO: Enable session tracking and test sessions payload property (Manually tested this, new ordering works correctly)

Review

For the submitter, initial self-review:

  • Commented on code changes inline explain the reasoning behind the approach
  • Reviewed the test cases added for completeness and possible points for discussion
  • A changelog entry was added for the goal of this pull request
  • Check the scope of the changeset - is everything in the diff required for the pull request?
  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

Cawllec and others added 30 commits June 6, 2018 17:00
- Add laravel test fixture for maze
- Add copy of testing.env to .env during build
@Cawllec Cawllec requested review from snmaynard and a team October 26, 2018 22:21
@Cawllec Cawllec changed the base branch from master to next October 26, 2018 22:22
Copy link
Contributor

@kattrali kattrali 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! It looks like the remaining tasks are to build out the tests to ensure the unhandled state is correct for errors generated from other places in the codebase and verifying session state (as mentioned in the description).

src/Middleware/UnhandledState.php Show resolved Hide resolved
@snmaynard
Copy link
Contributor

CI is failing.

@Cawllec Cawllec merged commit 5ce127c into next Nov 1, 2018
@Cawllec Cawllec deleted the cawllec/handled-state branch November 1, 2018 10:34
Cawllec added a commit that referenced this pull request Nov 2, 2018
* Dockerisation: Added dockerfile and updated Readme (#316)

* Dockerisation: Added dockerfile and updated Readme

* Dockerisation: Added composer installation into docker build

* Dockerisation: More example routes/navigation

* Dockerisation:  Updated wording/flow based on feedback

* Dockerisation: Streamlined dockerfile based on feedback

* Dockerisation: Added keygen

* Add correct handled/unhandled state to notifications (#325)

* handledState: Add Laravel Un/handled state checking

* Apply fixes from StyleCI (#303)

* handledState: Add Laravel Un/handled state checking
- Add laravel test fixture for maze

* Apply fixes from StyleCI (#304)

* handledState: Add Laravel Un/handled state checking
- Add copy of testing.env to .env during build

* feature: Update maze tests

* feature: Add un/handled middleware to client

* Apply fixes from StyleCI (#309)

* feature: Removed superfluous unit test for unhandledMiddleware

* handledState: Removed maze-tests to separate branch

* maze-tests: Initial maze fixture commit

* Apply fixes from StyleCI (#319)

* Handled state: Removed gemfile

* Maze-tests: Rebuilt laravel fixture

* Maze-tests: Added gemfile and git exceptions

* Maze-tests: Added supporting maze steps and environment

* Maze-tests: Add unhandled maze feature

* Apply fixes from StyleCI (#320)

* handledState: Ensure callable  is received and called

* HandledState: fixed desired class names

* Maze-tests: Moved shared server steps into maze-runner

* Maze tests: Pushed current changes

* Install bugsnag dep from local archive

* Maze tests:  Added dependencies to package composer.json in fixture

* HandledState: Made necessary change to middleware order

* MazeTest: Required new steps from maze, qualified gitignore

* MazeTests: Added further steps to unhandled feature

* Maze-tests: Added auto copying of bugsnag-laravel requirements into fixture

* Handled State: Ensure / are escaped correctly, add dependency on PHP changes

* Handled state: Add basic handled-state tests

* Apply fixes from StyleCI (#324)

* HandledState:  Added middleware methodology comment block

* HandledState: Added scenarios for checking session counts

* Apply fixes from StyleCI (#326)

* v2.15.0-alpha-1

* HandledState: Added handled and unhandled tests for several different places in Laravel apps

* Apply fixes from StyleCI (#327)

* HandledState: Fixed fixture route typo

* v2.15.0

* DockerDelay: Added delay before pings after docker-composer up (#329)

* Revert "DockerDelay: Added delay before pings after docker-composer up (#329)" (#330)

This reverts commit 35f319b.

* v2.15.0: Bumped changelog date
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.

None yet

3 participants