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

Use Axios's toJSON method to make shorter errors (#632) #662

Merged
merged 1 commit into from
Mar 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ Format:
### Security
-
-->
<!-- ## [Unreleased] -->
## [Unreleased]
### Changed
- Shorten Axios errors to make them more readable (https://github.com/SuffolkLITLab/ALKiln/pull/632)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the issue number isn't what you intended?

Suggested change
- Shorten Axios errors to make them more readable (https://github.com/SuffolkLITLab/ALKiln/pull/632)
- Shorten Axios errors to make them more readable (https://github.com/SuffolkLITLab/ALKiln/pull/605)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also liked the list of changes in the comment so I put them here, but if you don't want to include that, that's fine. Just an alternative version.

Suggested change
- Shorten Axios errors to make them more readable (https://github.com/SuffolkLITLab/ALKiln/pull/632)
- Shorten Axios errors to make them more readable (https://github.com/SuffolkLITLab/ALKiln/pull/605)
- Actually fail when we log the errors
- Move `toJSON()` calls to docassemble_api_REST
- Add better comment to `process.exitCode`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do intentionally link to the PR when I fix things. Issues usually have a lot of extra discussion that don't get implemented, and the issue is still linked to from the PR, usually in the first comment. I've found it's a lot easier to find the issue fixed from a PR than finding the PR from the issue.

I normally would be fine with adding more details to the changelog, but those specific commit comments are very internal, and don't make sense outside the context of the choices made in the PR ("move toJSON calls" move them from where? from a different file they were in earlier in the PR. From the changelog comment, it looks like those toJSON calls were around before that release, but they weren't).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Those make some sense. I'm good with whatever you choose.


## [4.10.4] - 2023-02-15
### Added
Expand Down
35 changes: 27 additions & 8 deletions lib/docassemble/docassemble_api_REST.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ da.get_dev_id = async function ( timeout ) {
timeout: timeout,
};

return await axios.request( options );
try {
return await axios.request( options );
} catch (error) {
throw error.toJSON();
}
}; // Ends da.get_dev_id()


Expand All @@ -41,8 +45,11 @@ da.create_project = async function ( project_name, timeout ) {
timeout: timeout,
};

return await axios.request( options );

try {
return await axios.request( options );
} catch (error) {
throw error.toJSON();
}
}; // Ends da.create_project()


Expand All @@ -61,7 +68,11 @@ da.pull = async function ( timeout ) {
timeout: timeout,
};

return await axios.request( options );
try {
return await axios.request( options );
} catch (error) {
throw error.toJSON();
}
}; // Ends da.pull()


Expand All @@ -78,7 +89,11 @@ da.has_task_finished = async function ( task_id, timeout ) {
timeout: timeout,
};

return await axios.request( options );
try {
return await axios.request( options );
} catch (error) {
throw error.toJSON();
}
}; // Ends da.has_task_finished()


Expand All @@ -95,7 +110,11 @@ da.delete_project = async function ( timeout ) {
timeout: timeout,
};

return await axios.request( options );
try {
return await axios.request( options );
} catch (error) {
throw error.toJSON();
}
}; // Ends da.delete_project()


Expand All @@ -121,9 +140,9 @@ da.__delete_projects_starting_with = async function ( base_name, starting_num=1,
timeout: 30 * 1000,
};

await axios.request( options );
await axios.request( options );
} catch ( error ) {
console.log( error )
console.log( error.toJSON() )
}
name_incrementor++;
}
Expand Down
7 changes: 1 addition & 6 deletions lib/docassemble/docassemble_api_interface.js
Original file line number Diff line number Diff line change
Expand Up @@ -250,9 +250,7 @@ da_i.loop_to_create_project = async function( creation_options ) {
});
throw ( error );
}

// ends if error.response
} else {
} else { // for if error.response

// If it's a timeout error, server is busy. Try again after waiting.
// Why are we using `max_tries` for timeout error as well?
Expand Down Expand Up @@ -435,12 +433,9 @@ da_i.delete_project = async function ( delete_options ) {
log.info({ pre: `Trying to delete Project ${ project_name }` });

try {

let response = await _da_REST.delete_project( timeout );
log.info({ pre: `Deleted Project "${ project_name }"` });

} catch ( error ) {
// Status 400
log.error({ pre: `Ran into an error when deleting Project "${ project_name }"` });
log.debug({ pre: `See https://docassemble.org/docs/api.html#playground_delete_project.` });
throw error;
Expand Down
12 changes: 11 additions & 1 deletion lib/docassemble/setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,17 @@ const setup = async () => {

// Create a project on the da server's account with a unique name then
// store it locally for later deleting the project.
let project_name = await da_i.loop_to_create_project();
let project_name = null;
// Explicit try-catch prevents "Unhandled promise rejection error"
// avoids showing confusing node errors to users that they shouldn't have to fix
try {
project_name = await da_i.loop_to_create_project();
} catch (error) {
log.error({type: `setup`, pre: `an unexpected error occurred while trying to create a docassemble project in the user's account`, data: JSON.stringify(error, null, 4)})
// we don't re-throw the exception, so make sure to exit with 1 to let Github Actions know the script failed
process.exitCode = 1;
return;
}
// Once the name is created, save that name to a local file.
session_vars.save_project_name( project_name );

Expand Down
14 changes: 11 additions & 3 deletions lib/docassemble/takedown.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,17 @@ const takedown = async () => {
* Project created for this test.
* For local development, clean up project name file. */
log.info({ type: `takedown`, pre: `Trying to remove the docassemble Project from the testing account.` });
await da_i.delete_project();
session_vars.delete_project_name();
log.info({ type: `takedown`, pre: `Success! Test takedown has finished successfully.` });
// Explicit try-catch prevents "Unhandled promise rejection error"
// avoids showing confusing node errors to users that they shouldn't have to fix
try {
await da_i.delete_project();
session_vars.delete_project_name();
log.info({ type: `takedown`, pre: `Success! Test takedown has finished successfully.` });
} catch (error) {
log.error({type: `takedown`, pre: `could not delete project`, data: JSON.stringify(error, null, 4)})
// we don't re-throw the exception, so make sure to exit with 1 to let Github Actions know the script failed
process.exitCode = 1;
}
};

takedown();