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

SF-2392 Add event metric logging #2894

Merged
merged 21 commits into from
Jan 15, 2025
Merged

SF-2392 Add event metric logging #2894

merged 21 commits into from
Jan 15, 2025

Conversation

pmachapman
Copy link
Collaborator

@pmachapman pmachapman commented Dec 9, 2024

This PR adds support for logging events via the [LogEventMetric] attribute.

To access it, a button is added to the serval admin page. Although permissions are granted for project admins to view the event log, a link is not provided in the UI until we are ready to expose it to a wider audience.

This PR is intended to be a minimal MVP leaving enough scope for us to expand in the coming months. Perhaps the bulk of the new code is unit tests for existing functionality that did not have unit tests.

See https://github.com/sillsdev/web-xforge/wiki/Event-Metric-Logging for documentation on how this feature works and how to use it.


This change is Reviewable

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.97%. Comparing base (450be10) to head (b8749b7).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2894      +/-   ##
==========================================
+ Coverage   80.89%   81.97%   +1.08%     
==========================================
  Files         533      545      +12     
  Lines       31262    31536     +274     
  Branches     5083     5112      +29     
==========================================
+ Hits        25288    25851     +563     
+ Misses       5211     4939     -272     
+ Partials      763      746      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pmachapman pmachapman force-pushed the feature/SF-2392 branch 3 times, most recently from 9042475 to 8c20c54 Compare December 11, 2024 21:02
@kylebuss kylebuss assigned kylebuss and unassigned kylebuss Dec 13, 2024
@pmachapman pmachapman force-pushed the feature/SF-2392 branch 2 times, most recently from 90e46eb to b6ec6c6 Compare December 15, 2024 23:35
Copy link
Collaborator

@kylebuss kylebuss left a comment

Choose a reason for hiding this comment

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

I'm not certain that both the event-metrics.component and event-metrics-log.component are necessary. Is there a reason for two separate components for this?

Reviewed 5 of 11 files at r1, 5 of 10 files at r2, 8 of 11 files at r3, 2 of 8 files at r4, 17 of 21 files at r6, all commit messages.
Reviewable status: 37 of 41 files reviewed, 9 unresolved discussions (waiting on @Github-advanced-security[bot] and @pmachapman)


src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metrics-log.component.ts line 35 at r6 (raw file):

  constructor(
    noticeService: NoticeService,
    private readonly i18n: I18nService,

Will this be needed since it's an admin only screen?

Code quote:

private readonly i18n: I18nService,

src/SIL.XForge/EventMetrics/EventScope.cs line 1 at r6 (raw file):

namespace SIL.XForge.EventMetrics;

Should we consider adding Users scope? For SF invites and roles?

Copy link
Collaborator Author

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

I'm not certain that both the event-metrics.component and event-metrics-log.component are necessary. Is there a reason for two separate components for this?

This PR is still work in progress. I am still writing this (and other) areas of the code.

Dismissed @Github-advanced-security[bot] from 7 discussions.
Reviewable status: 37 of 41 files reviewed, 2 unresolved discussions (waiting on @Github-advanced-security[bot] and @kylebuss)


src/SIL.XForge/EventMetrics/EventScope.cs line 1 at r6 (raw file):

Previously, kylebuss (Kyle Buss) wrote…

Should we consider adding Users scope? For SF invites and roles?

These can be added if needed.


src/SIL.XForge.Scripture/ClientApp/src/app/event-metrics/event-metrics-log.component.ts line 35 at r6 (raw file):

Previously, kylebuss (Kyle Buss) wrote…

Will this be needed since it's an admin only screen?

Yes, as project admins will access it.

@pmachapman pmachapman force-pushed the feature/SF-2392 branch 6 times, most recently from e7f66e8 to 42ab49d Compare December 19, 2024 00:47
@pmachapman pmachapman force-pushed the feature/SF-2392 branch 2 times, most recently from bb24575 to 78f2718 Compare December 19, 2024 02:58
@kylebuss
Copy link
Collaborator

src/SIL.XForge/EventMetrics/EventScope.cs line 1 at r6 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

These can be added if needed.

nit Can the EventScope be renamed to AppComponent or AppFeature? I think this would be useful for the Help Videos and would like to use the enum.

@pmachapman pmachapman force-pushed the feature/SF-2392 branch 4 times, most recently from 3665ea1 to e0a3927 Compare January 6, 2025 02:47
Copy link
Collaborator

@kylebuss kylebuss left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r17, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)

Copy link
Collaborator

@kylebuss kylebuss left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r18, 6 of 6 files at r19, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)

@Nateowami Nateowami added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test labels Jan 15, 2025
@Nateowami Nateowami enabled auto-merge (squash) January 15, 2025 15:36
@Nateowami Nateowami merged commit c433724 into master Jan 15, 2025
14 of 15 checks passed
@Nateowami Nateowami deleted the feature/SF-2392 branch January 15, 2025 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing complete Testing of PR is complete and should no longer hold up merging of the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants