Some security history events appear un-rendered

[di]

It appears that some security history events are being included at https://pypi.org/manage/account/ which aren't rendered into something human readable, specifically account:role:add and account:role:invite:

Here's how we display these for the Project event history:

https://github.com/pypi/warehouse/blob/05225e5329c8747317eabca630002e11a26a7d09/warehouse/templates/manage/project/history.html#L132-L135

https://github.com/pypi/warehouse/blob/05225e5329c8747317eabca630002e11a26a7d09/warehouse/templates/manage/project/history.html#L153-L156

And here's where we should this in the account security history section:

https://github.com/pypi/warehouse/blob/05225e5329c8747317eabca630002e11a26a7d09/warehouse/templates/manage/account.html#L476-L673

[ewdurbin]

Wonder if we could implement a common include for rendering events generically?

By consolidating all of the disparate renders into one it would map to events/tags.py well also.

[TheShiveshNetwork]

@di Is this issue still open? If so can I start working on the fix?

[di]

This issue is still open.

[jzohrab]

Taking a quick look at this. Double-checking the branches:

The if/elif checks in warehouse/templates/manage/project/history.html contain all of the values in class Project(EventTagEnum). (e.g. {% elif event.tag == EventTag.Project.APITokenRemoved %}). No change needed there.

The if/elif checks in warehouse/warehouse/templates/manage/account.html are missing the following cases:

class EventTag:
    class Account(EventTagEnum):
        """Tags for User events."""

        # Name = "source_type:subject_type:action"
        OrganizationRoleAdd = "account:organization_role:add"
        OrganizationRoleChange = "account:organization_role:change"
        OrganizationRoleRemove = "account:organization_role:remove"
        PasswordDisabled = "account:password:disabled"
        RoleAdd = "account:role:add"            ####### Noted in the issue description above
        RoleChange = "account:role:change"
        RoleDeclineInvite = "account:role:decline_invite"
        RoleInvite = "account:role:invite"        ####### Noted in the issue description above
        RoleRemove = "account:role:remove"
        RoleRevokeInvite = "account:role:revoke_invite"
        TeamRoleAdd = "account:team_role:add"
        TeamRoleRemove = "account:team_role:remove"
        TwoFactorDeviceRemembered = "account:two_factor:device_remembered"

@di - should all of the above cases also be covered in the if/elif branches?

I'm not sure what info would be rendered for each of these cases, but we could make a draft PR at least with these branches added and an initial obvious guess (e.g., <strong>{% trans %}Password disabled{% endtrans %}</strong>)

(Side note: Having the data and rendering coded across a few places feels brittle, but it would probably be a lot of work with minimal payoff to refactor.)

[di]

@di - should all of the above cases also be covered in the if/elif branches?

Ideally, yes. I think your plan sounds good!

[jzohrab]

Thanks @di - a few more questions:

1. the template has {% elif event.tag == "account:reauthenticate:failure" %}, but the reauth constant is commented out # ReauthenticateFailure = "account:reauthenticate:failure" -- should it be reactivated?
2. should we sort the tags constants to alphabetical and reorganize the elif branches so that they're in the same order as the tag constants? Very minor point!

[jzohrab]

Actually, scratch point 2 above for now, which would complicate the PR code review. And I'll not change anything re point 1 either, keeping the upcoming PR more focused. We can do those points in a separate PR if needed/wanted.

[di]

the template has {% elif event.tag == "account:reauthenticate:failure" %}, but the reauth constant is commented out # ReauthenticateFailure = "account:reauthenticate:failure" -- should it be reactivated?

It should not be reactivated. We have some events (there are more than this one) which we no longer emit but may exist in security history, so we still need to render them.