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

Feature: Model for statistics #205

Merged
merged 20 commits into from
Jan 17, 2023
Merged

Conversation

antonykamp
Copy link
Collaborator

@antonykamp antonykamp commented Jan 4, 2023

Fixes #138

This PR introduces audit event logs, to collect data of items for future statistics.
We can improve the performance in the future by calculating results of metrices on a regular basis with a cronjob.

PR checklist

Adds test for creating audit events to existing test infrastructure.
Adds the model, migration and factory and updates the schema. Additionally, it introduces a "has_many" connection to the item model. It destroys the item with the audit events, because they won't be relevant anymore.
Introduces the audit event helper as interface for audit event. User are aware of available audit event types now.
Adds the creation of audit events to the existing infrastructure (mostly in item controller). Therefore, I had to disable the rubocop abc-metric for a function, because I couldn't reduce the complexity.
Introduces empty statistics helper to add functions for calculating statistics and metrices in the future.
antonykamp and others added 2 commits January 4, 2023 07:19
I had to expand disabling the rubocop abc-metric for another function, because I couldn't reduce the complexity.
Copy link
Collaborator

@SaturnHafen SaturnHafen left a comment

Choose a reason for hiding this comment

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

Looks great so far! Especially the simple interface for adding a new audit event. I personally would test for more properties in the tests (as proposed below).
Additionally, I don't fully understand why some fields in the data-model are necessary.

app/controllers/items_controller.rb Outdated Show resolved Hide resolved
app/models/audit_event.rb Outdated Show resolved Hide resolved
app/models/item.rb Show resolved Hide resolved
spec/features/items/show.html.erb_spec.rb Outdated Show resolved Hide resolved
spec/models/audit_event_spec.rb Outdated Show resolved Hide resolved
spec/features/items/show.html.erb_spec.rb Outdated Show resolved Hide resolved
spec/features/items/show.html.erb_spec.rb Outdated Show resolved Hide resolved
spec/features/items/show.html.erb_spec.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@Lietze Lietze left a comment

Choose a reason for hiding this comment

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

Found one more little thing.

spec/models/audit_event_spec.rb Outdated Show resolved Hide resolved
spec/models/audit_event_spec.rb Outdated Show resolved Hide resolved
spec/models/audit_event_spec.rb Outdated Show resolved Hide resolved
spec/models/audit_event_spec.rb Outdated Show resolved Hide resolved
spec/models/audit_event_spec.rb Outdated Show resolved Hide resolved
spec/models/audit_event_spec.rb Outdated Show resolved Hide resolved
@antonykamp
Copy link
Collaborator Author

I'll clean the commit history with the squash 😇 I'm sorry for the mess

Copy link
Contributor

@LucasDerReisende LucasDerReisende left a comment

Choose a reason for hiding this comment

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

Description of Functionality looks good to me! Keep up the good work team!

Copy link
Collaborator

@SaturnHafen SaturnHafen left a comment

Choose a reason for hiding this comment

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

Looks good! I haven't confirmed if there are conflicts with @Greenscreen23's changes on dev. We should probably check that.

app/helpers/audit_event_helper.rb Outdated Show resolved Hide resolved
app/controllers/items_controller.rb Outdated Show resolved Hide resolved
app/helpers/audit_event_helper.rb Outdated Show resolved Hide resolved
@antonykamp
Copy link
Collaborator Author

Thank you very much for the review again @SaturnHafen It's very helpful 😇
So you recomment to remove everything (test, audit_event) that is connected to the old deny_return event, correct?

Copy link
Contributor

@LucasDerReisende LucasDerReisende left a comment

Choose a reason for hiding this comment

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

Looks great! Keep up the good work team!!🎉

Copy link
Collaborator

@SaturnHafen SaturnHafen left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for all the changes, and sorry for being so picky :)

@antonykamp antonykamp merged commit edd4b53 into dev Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BP-AP BP Polze
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model for statistics
4 participants