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 response.statusMessage data information in environment logs #853

Merged
merged 3 commits into from Dec 1, 2022
Merged

Add response.statusMessage data information in environment logs #853

merged 3 commits into from Dec 1, 2022

Conversation

AndreiOrmanji
Copy link
Contributor

@AndreiOrmanji AndreiOrmanji commented Nov 2, 2022

Closes #852

Technical implementation details
I've been added a new property statusMessage in Transaction.response located in packages\commons\src\models\server.model.ts for getting the statusMessage in "Logs" tab and some other places:

  • function CreateTransaction in packages\commons-server\src\libs\utils.ts that converts a pair of Request and Response objects in a Transaction object
  • EnvironmentLogResponse in packages\desktop\src\renderer\app\models\environment-logs.model.ts for showing the information from modified Transaction in "Logs" tab
  • function formatLog in packages\desktop\src\renderer\app\services\data.service.ts for converting the Transaction object in
    EnvironmentLogResponse object.
  • added a new item in "General" section of "Response" tab in packages\desktop\src\renderer\app\components\environment-logs\environment-logs.component.html
  • updated existing tests for covering my changes.

Disclaimer:
I'm not an experienced open software contributor or at least a JS developer, but I want to help you, Mockoon supporters, to make Mockoon better.

Checklist

  • data migration added (@mockoon/commons)
  • data migration automated tests added (@mockoon/commons)
  • CLI automated tests added (@mockoon/cli)
  • desktop automated tests added (@mockoon/desktop)

@AndreiOrmanji
Copy link
Contributor Author

I don't know if I'm allowed to mark any item in checklist as "solved", so I've left it all "as is".

Copy link
Member

@255kb 255kb left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.
I run the CI and there are some Prettier formatting issue. Please run Prettier before pushing again. I also add a comment concerning the log label.

@255kb
Copy link
Member

255kb commented Nov 7, 2022

Now that I am thinking about it, I think it would be better to display the Status message next to the status code, so it's more in line with what other tools like Curl are displaying. What do you think?

@AndreiOrmanji
Copy link
Contributor Author

AndreiOrmanji commented Nov 7, 2022

Now that I am thinking about it, I think it would be better to display the Status message next to the status code, so it's more in line with what other tools like Curl are displaying. What do you think?

In my optinion,it will "break existing UI consistency" where you have already 'General', 'Headers', 'Body' sections.
If you are ok with adding more details, I can add HTTP version, sent in response by server (probably can be useful for other users).

@255kb
Copy link
Member

255kb commented Nov 8, 2022

Now that I am thinking about it, I think it would be better to display the Status message next to the status code, so it's more in line with what other tools like Curl are displaying. What do you think?

In my optinion,it will "break existing UI consistency" where you have already 'General', 'Headers', 'Body' sections. If you are ok with adding more details, I can add HTTP version, sent in response by server (probably can be useful for other users).

What I meant was to add the message next to the status code to avoid adding a new line, like this:

Status: 404 - Not Found

Instead of:

Status: 404
Status message: Not Found

I'm not sure if adding the HTTP version is really useful. Wouldn't it be always something like 1.1 as we are using Node.js' http?

@AndreiOrmanji
Copy link
Contributor Author

AndreiOrmanji commented Nov 9, 2022

Now that I am thinking about it, I think it would be better to display the Status message next to the status code, so it's more in line with what other tools like Curl are displaying. What do you think?

In my opinion, it will "break existing UI consistency" where you have already 'General', 'Headers', 'Body' sections. If you are ok with adding more details, I can add HTTP version, sent in response by server (probably can be useful for other users).

What I meant was to add the message next to the status code to avoid adding a new line, like this:

Status: 404 - Not Found

Instead of:

Status: 404
Status message: Not Found

Your varians looks good too :)
If you want, I can make some changes, just say how you would like to see it.

I'm not sure if adding the HTTP version is really useful. Wouldn't it be always something like 1.1 as we are using Node.js' http?

It depends on server the requests are sent to. For example, persistent connection (read more at https://developer.mozilla.org/en-US/docs/Web/HTTP/Connection_management_in_HTTP_1.x) is available from 1.1 and higher, so it can be useful to know what version is supported by server.

@255kb
Copy link
Member

255kb commented Nov 9, 2022

If you don't mind, maybe display status code and message on one line, I think I prefer 🙂
Also, if you want to display the HTTP version go for it!

@AndreiOrmanji
Copy link
Contributor Author

If you don't mind, maybe display status code and message on one line, I think I prefer 🙂 Also, if you want to display the HTTP version go for it!

I want to add HTTP version. Can you add an example how it would look like?

@255kb
Copy link
Member

255kb commented Nov 9, 2022

Maybe something like:

Protocol: HTTP1.1
Status: 404 - Not found

So it's ready for other protocols like web socket when we will implement web sockets 😄.

@AndreiOrmanji
Copy link
Contributor Author

Maybe something like:

Protocol: HTTP1.1
Status: 404 - Not found

So it's ready for other protocols like web socket when we will implement web sockets smile.

I've implemented like so:
image.

@AndreiOrmanji
Copy link
Contributor Author

Sorry for pipeline fails, but I could not set up project on my machine to execute tests from pipeline...
Fixed problem caused pipeline fail.

@255kb
Copy link
Member

255kb commented Nov 15, 2022

No worries, I used the CI all the time to run the tests 😅
There are still many test that fail, apparently for an extra ":".
You can run the test locally by running first npm run build:desktop:ci then npm run test:desktop -- --spec "environment-logs.spec.ts".
Let me know if you need help with anything.

@AndreiOrmanji
Copy link
Contributor Author

It seems that node js and express did not provide access to protocol and version to send in response, so I've set values from request, where it was possible.
I would remove Protocol section because i'm not sure it was implemented in a "correct" way and to be implemented by someone who has more experience with JS and node.

I've refused to use docker. With windows I've the following error on test running:

$ npm -v
8.19.2

$ node -v
v19.0.0

$ nvm -v
Running version 1.1.9.

Screenshot:
image

@255kb
Copy link
Member

255kb commented Nov 30, 2022

I would remove Protocol section because i'm not sure it was implemented in a "correct" way and to be implemented by someone who has more experience with JS and node.

I would say the same. Maybe remove it and stick to the status message only? It's probably too much hassle for the benefits right now.

- fixed formatting in packages/desktop/src/renderer/app/components/environment-logs/environment-logs.component.html
- changed label for response.statusMessage
- updated tests with current fixes

Closes #852
@AndreiOrmanji
Copy link
Contributor Author

Maybe remove it and stick to the status message only? It's probably too much hassle for the benefits right now.

Removed changes, related to Protocol, so the current version looks like:
image

@255kb
Copy link
Member

255kb commented Dec 1, 2022

Thank you for the changes. Everything looks ok, I will merge and add it to the next release (probably later this month).

@AndreiOrmanji
Copy link
Contributor Author

Thank you very much

@255kb 255kb merged commit 48fb1c7 into mockoon:main Dec 1, 2022
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.

Add response.statusMessage data information in environment logs
2 participants