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

Test webhooks & fixes #48

Merged
merged 13 commits into from
Dec 4, 2020
Merged

Test webhooks & fixes #48

merged 13 commits into from
Dec 4, 2020

Conversation

johnaohara
Copy link
Member

This PR introduces a number of changes, fixes and enhancements;

  • Allow users to register webhooks for "change/new" events
  • Allow users to register webhooks for individual tests.
  • Fix podman guidance
  • Add OpenAPI endpoint
  • Enable Redux Devtools in dev mode
  • Allow users to rename existing tests
  • Allow users to define variable names that contain reservered SQL characters. e.g. 'some-value'

@johnaohara johnaohara self-assigned this Dec 4, 2020
@johnaohara johnaohara merged commit 1316c4b into Hyperfoil:master Dec 4, 2020
Copy link
Member

@rvansa rvansa left a comment

Choose a reason for hiding this comment

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

I think that once you've exposed per-test hooks in the Tests view you should remove them from the global tab.
I wonder if we should change the type of Hook.target from anonymous integer straight to Test and enforce referential integrity in the database.

}}
aria-label="Event Type"
>
{eventTypes.map((option, index)=>{
Copy link
Member

Choose a reason for hiding this comment

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

So you can select test/new here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, that doesn't make sense, will fix that

setTestWebHooks(response.map((h: Hook) => {
let hd: Hook = {
...h,
id: Number(h.id),
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering, why are those explicit type casts needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

idk if they are explicitly required, iirc the IDE was not resolving the correct type but that is probably an issue with the IDE rather than the type could not be inferred at runtime. I can look at removing the type cast

Copy link
Member Author

Choose a reason for hiding this comment

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

response array is result of a call to fetchApi() which returns a Promise<Any>. so response.map((h: Hook) requires the typecast, unless we want to include return type as a param to fetchApi(). That would be quite an intrusive change. Explicitly casting let hd: Hook can be dropped.

@@ -129,6 +136,10 @@ export const reducer = (state = new TestsState(), action: TestAction) => {
state.byId = state.byId?.set(action.testId, { ...test, defaultView: action.view })
}
}
break;
case actionTypes.UPDATE_HOOK: {
//TODO: define state changes
Copy link
Member

Choose a reason for hiding this comment

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

I must admit that I am not entirely sold on storing a cached copy of the database in the browser, especially when a part of the object might not be loaded (and there's not a clear distinction between not-loaded and not-set-at-all from the typesystem POV), but we should be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

This TODO comment might be out of date. I thought the whole premise of redux is to enable easy tracking of state (which involves caching on client side), otherwise you need to manage gui state CRUD operations manually

@johnaohara
Copy link
Member Author

I think that once you've exposed per-test hooks in the Tests view you should remove them from the global tab.
I wonder if we should change the type of Hook.target from anonymous integer straight to Test and enforce referential integrity in the database.

I did think about this, but there might be a use case where a tool might want to register for all events of a particular type. Also, the "test/new" event is fired when a new test is created, so in order to respond to those events that could not be related to an individual test.

@jesperpedersen jesperpedersen mentioned this pull request Jun 12, 2023
@johnaohara johnaohara deleted the test-webhooks branch October 5, 2023 13:54
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.

2 participants