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/user event log #365

Merged
merged 50 commits into from
May 18, 2022
Merged

Feat/user event log #365

merged 50 commits into from
May 18, 2022

Conversation

mattyg
Copy link
Collaborator

@mattyg mattyg commented May 5, 2022

Resolves #309
Resolves #375

Overview

  • new api endpoint /api/eventlogs/all
  • helper utility logEvent()
  • tests on api endpoint & helper utility
  • Frontend for viewing list of events, paginated
  • Add logEvent() calls everywhere that a user event occurs (see list above)
  • Refactor logic to determine a user's display name: move from frontend to backend, attach to UserDto as nameRealized

To limit the scope for this PR, I did not include custom filtering / searching / sorting of event logs. (I created another ticket #376). I also did not include infinite-scroll. Let me know if you think its preferable to just build out that functionality as part of this PR.

--

Our outline of this feature in #309 was kinda fuzzy, so I tried to think it through and decide on what to include in this UserEventLog data. This is all open to feedback and revision!

Here's what I came up with:

  • The purpose of this log is user-facing community transparency, not systems administration nor debugging.
  • For that purpose, the only important data is "changes that someone made to the database state"
  • Thus, user-interaction commands which do not change the database state should not be logged.
  • Thus, user actions which "fail", and do not change the database state, should not be logged

So the only data included in this log are the following user actions which change the database state:

  • Log in
  • Create Period
  • Update Period
  • Close Period
  • Assign random quantifiers to all praise in period
  • Update Period Setting
  • Update Global Setting
  • Add role to user
  • Remove role from user
  • Quantify praise
  • Run /activate command on discord (initiate UserAccount activation)
  • Create Forwarded Praise
  • Create Praise
  • Replace quantifiers (cli command)
  • Import praise (cli command)

This does not account for changes made by a sysadmin directly to the database itself (i.e. bypassing the application interfaces).

Also, just a note that currently makes individual quantification scores public, before the quantification period is closed. Perhaps it should be modified to only include quantification user events after the related period is closed.

mattyg added 25 commits May 5, 2022 14:08
…y, update entities, migration, and helper util to reflect
…nt, as actions can be performed by both users and useraccounts (see #361)
…er actions from quantifier actions (as there will be a lot of both)
@mattyg mattyg changed the title WIP Feat/user event log Feat/user event log May 11, 2022
@mattyg mattyg requested review from kristoferlund and nebs-dev and removed request for kristoferlund and nebs-dev May 11, 2022 01:15
@mattyg mattyg requested review from kristoferlund and nebs-dev May 11, 2022 17:39
@kristoferlund kristoferlund changed the base branch from dev to main May 12, 2022 08:25
Copy link
Member

@kristoferlund kristoferlund 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!! ✨

One additional action we should log is:

  • Remove role from user

For the layout, in general we should try keep it as close to the other pages as possible. These are my proposed changes:

image

@mattyg
Copy link
Collaborator Author

mattyg commented May 14, 2022

  • Remove role from user

done

For the layout, in general we should try keep it as close to the other pages as possible. These are my proposed changes:

done

With this UX change, I had to add some additional conditionals to determine which user name to store. There will very likely be weird / inconsistent bugs with the displayed user name (until we implement #361 and refactor out the confusion around users and useraccounts).

Currently, some actions are taken by useraccounts (sometimes without an associated user), while others are taken by users. Thus the eventlog model has to store both the user id and / or the useraccount id. Thus we need to conditionally determine the user name to display based on the data available within each individual event log.

Also I just wanted to make sure you saw this from the comment above. Let me know if you think it requires any changes:

Also, just a note that currently makes individual quantification scores public, before the quantification period is closed. Perhaps it should be modified to only include quantification user events after the related period is closed.

@mattyg mattyg requested review from kristoferlund and removed request for nebs-dev May 14, 2022 19:33
Copy link
Member

@kristoferlund kristoferlund 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!

The only thing I noticed is that the "← Previous" page link don't appear.

Currently, some actions are taken by useraccounts (sometimes without an associated user), while others are taken by users. Thus the eventlog model has to store both the user id and / or the useraccount id. Thus we need to conditionally determine the user name to display based on the data available within each individual event log.

Yes. We need to clear this up and decide on a model that is more sustainable. Let's continue discussion in #361.

Also, just a note that currently makes individual quantification scores public, before the quantification period is closed. Perhaps it should be modified to only include quantification user events after the related period is closed.

Sounds like a good idea. If I remember correctly the rule is this:

  • Admins can see everything
  • Users and Quantifiers can only see scores after period has been closed

@mattyg
Copy link
Collaborator Author

mattyg commented May 17, 2022

The only thing I noticed is that the "← Previous" page link don't appear.

Fixed

Also, just a note that currently makes individual quantification scores public, before the quantification period is closed. Perhaps it should be modified to only include quantification user events after the related period is closed.

Sounds like a good idea. If I remember correctly the rule is this:

  • Admins can see everything
  • Users and Quantifiers can only see scores after period has been closed

Done. Quantification event logs for an active period are still included, but their description is obscured by a Notice that says "Praise scores are not visible during quantification" (the same notice as in the PeriodDetailPage).

I think it makes sense to include them with a notice hiding the details rather than to exclude them entirely. But let me know what you think:

Screenshot_2022-05-17_16-53-52

@mattyg mattyg requested a review from kristoferlund May 17, 2022 21:00
Copy link
Member

@kristoferlund kristoferlund 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!

)}`;
};

export const generateUserName = async (user: UserDocument): Promise<string> => {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, like we talked about, attempting to establish a single source of truth for every business rule. And to push it as far up the "tree" as possible with the database representing the "root node".

@kristoferlund kristoferlund merged commit 56c1575 into main May 18, 2022
@kristoferlund kristoferlund deleted the feat/user_event_log branch May 18, 2022 09:26
mattyg added a commit that referenced this pull request May 23, 2022
…ed (reflect changes in #365), switch to using shortenEthAddress implementation from api.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to change endDate of last period System activity log
2 participants