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

feat(audittrail): now audit trail can be configured by mode or output type to ignore some fields #92

Merged
merged 7 commits into from
Mar 6, 2023

Conversation

jhomarolo
Copy link
Contributor

@jhomarolo jhomarolo commented Feb 14, 2023

Fixes and changes #90

Proposed Changes

  1. fix ci
  2. now audit trail can be configured by mode or output type to ignore some fields
  3. improve coverage

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • Remember to check if code coverage decrease, if so, please implement the necessary tests

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

… type to ignore some fields

You can also configure the output to ignore some fields of the audit trail by choosing those options
(mode or output)

herbsjs#90
@jhomarolo jhomarolo added enhancement New feature or request severity-major Item is very important labels Feb 14, 2023
@jhomarolo
Copy link
Contributor Author

jhomarolo commented Feb 14, 2023

Suggestions are welcome! 😃

README.md Outdated

```javascript
auditTrail: {
mode: "minimal", // if minimal won't output especific fields: steps, request, return and user
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest starting without a "shortcut" like mode: "minimal" , leaving to future improvements.

Any assumptions we make now about this shortcuts now will probably be wrong.

This feature can be achieved by the user using config.auditTrail.mininal, where:

config.auditTrail = { mininal: { ... } }
const uc = usecase('A use case', {
                auditTrail: config.auditTrail.mininal

@@ -85,6 +91,33 @@ class UseCase {
}

get auditTrail() {

Copy link
Member

Choose a reason for hiding this comment

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

The way it was implemented, the audit is collected but discarded at the end, when auditTrail() is called. I would suggest not collect the data at all, changing each audit line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your point, but I think that adding these validations at the time of collection will make the code more complex, which could be an improvement in the future if there is any clear performance record.

src/usecase.js Outdated
@@ -85,6 +91,33 @@ class UseCase {
}

get auditTrail() {

if(this._mainStep.auditTrail.configuration && this._mainStep.auditTrail.configuration?.mode === "minimal")
Copy link
Member

Choose a reason for hiding this comment

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

Since is is used so many times, this._mainStep.auditTrail.configuration can be stored in a variable, for a cleaner code

README.md Outdated
```javascript
auditTrail: {
mode: "minimal", // if minimal won't output especific fields: steps, request, return and user
output:{
Copy link
Member

Choose a reason for hiding this comment

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

The output is really necessary? Could it be just auditTrail: { steps: false } (for ex)?

Copy link
Contributor Author

@jhomarolo jhomarolo Feb 26, 2023

Choose a reason for hiding this comment

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

I put output precisely to make it more evident what those true or false variables would be. Precisely so that there are no breaks in future developments. But since we removed minimal, I also remove the output

@codecov
Copy link

codecov bot commented Feb 26, 2023

Codecov Report

Merging #92 (8d3d295) into master (833b005) will increase coverage by 0.50%.
The diff coverage is 100.00%.

❗ Current head 8d3d295 differs from pull request most recent head c9db42d. Consider uploading reports for the commit c9db42d to get more accurate results

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   97.64%   98.14%   +0.50%     
==========================================
  Files           7        7              
  Lines         255      270      +15     
==========================================
+ Hits          249      265      +16     
+ Misses          6        5       -1     
Impacted Files Coverage Δ
src/request.js 100.00% <100.00%> (ø)
src/usecase.js 97.14% <100.00%> (+0.77%) ⬆️
src/results.js 94.73% <0.00%> (+2.63%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jhomarolo jhomarolo requested a review from dalssoft March 2, 2023 13:01
@dalssoft
Copy link
Member

dalssoft commented Mar 3, 2023

@jhomarolo, all fine to merge! great contribution!

could you open a issue (and a PR) to document this new behavior? tks

@herbsjs-robot
Copy link

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

jhomarolo added a commit to jhomarolo/herbs that referenced this pull request Mar 6, 2023
Now buchu have the auditTrail configuration and user inside the context variable

BREAKING CHANGE: Projects that are using ctx.user. A ctx.user variable will now be overridden by
context. So now this variable becomes reserved by the library and we recommend not using it beyond
the purpose of capturing user information coming via authorize

re herbsjs/buchu#92
herbsjs-robot pushed a commit to herbsjs/herbs that referenced this pull request Mar 6, 2023
# [2.0.0](v1.6.2...v2.0.0) (2023-03-06)

### Features

* **buchu:** update buchu version to 2.1.0 ([d038691](d038691))

### BREAKING CHANGES

* **buchu:** Projects that are using ctx.user. A ctx.user variable will now be overridden by
context. So now this variable becomes reserved by the library and we recommend not using it beyond
the purpose of capturing user information coming via authorize

re herbsjs/buchu#92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released severity-major Item is very important
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants