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

Option to configure number of saved responses to history #1378

Merged

Conversation

Projects
None yet
2 participants
@Grimones
Copy link
Contributor

commented Feb 20, 2019

Adds and option to configure how many responses it should save to history.

Today I got in a situation where I sent a malformed request to 3rd-party API and it accidentally cleared crucial data. And I couldn't find the response\url because at the time we've noticed that something was wrong the history was already overridden.

@welcome

This comment has been minimized.

Copy link

commented Feb 20, 2019

💖 Thanks for opening this pull request! 💖

To help make this a smooth process, please be sure you have first read the
contributing guidelines.

@Grimones

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

The OAuth 2.0 are breaking. Not sure why 😕

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Mar 11, 2019

Don't worry, it's complicated 😭. It's because the tests use deterministic ID generation logic, so the addition of the models.settings.getOrCreate() throws it off by adding an additional ID into the mix.

My proposed changes should make the tests pass again.

@gschier
Copy link
Collaborator

left a comment

Thanks for this change @Grimones! Just one comment before we can move forward 😄

@@ -130,13 +129,19 @@ export async function create(patch: Object = {}) {

const { parentId } = patch;

const settings = await models.settings.getOrCreate();

This comment has been minimized.

Copy link
@gschier

gschier Mar 12, 2019

Collaborator

In order to decouple this a bit, I think it'd be wise to pass the max responses into the create function as a parameter instead of looking up the settings object from there. I would add it as a second, optional, parameter to keep things simpler. Note, only the calls within app.js would need to be updated.

export async function create(patch: Object = {}, maxResponses: number = 20) {
  // ...
}
@gschier
Copy link
Collaborator

left a comment

Going to merge this one in and clean it up on my end. Thanks for the contribution! 😄

@gschier gschier merged commit e145ca8 into getinsomnia:develop Apr 18, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@welcome

This comment has been minimized.

Copy link

commented Apr 18, 2019

Congrats on merging your first pull request! 🎉🎉🎉 You're helping make Insomnia awesome! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.