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

Save additional author meta as Stream meta for every record #389

Closed
fjarrett opened this issue Apr 6, 2014 · 15 comments
Closed

Save additional author meta as Stream meta for every record #389

fjarrett opened this issue Apr 6, 2014 · 15 comments

Comments

@fjarrett
Copy link
Contributor

fjarrett commented Apr 6, 2014

While discussing some charting data issues with @Chacha in https://github.com/x-team/wp-stream-reports/issues/69 it became apparent to me that the current way Stream is serving up author data is lacking.

For instance, if a user is removed from WordPress, any records authored by them will end up looking like this:

screen shot 2014-04-06 at 12 56 20 am

The same issue applies to roles that may be removed from WordPress. Furthermore, if a user's role changes, their current role will always be displayed, instead of the role they had when the record was actually created.

If the philosophy of Stream is to save snapshots of WP Admin actions as they happen, then I think our method right now - serving up current data about the author - makes the record historically inaccurate.

We have already solved this problem for the object of a record by saving additional data about that object as Stream meta (sidebar_name, post_title, plugin version number, etc.). But for data about the author, we are relying solely on the user_ID and fetching the other author meta we need from the current state of WordPress.

So my question is, what do you guys think about including additional author meta as Stream meta for every new record?

  • user_email (for gravatars)
  • user_display_name
  • user_login (fallback if no display_name is set)
  • user_role (role slug)
  • user_role_label (translated label for the role)

I think that logging this additional user meta would give us everything we need to have an historically accurate snapshot of the author's state when they performed the action.

@lukecarbis
Copy link
Contributor

👍 Sounds good to me. Would you consider storing them all as a single serialized entry?

@fjarrett
Copy link
Contributor Author

fjarrett commented Apr 6, 2014

@lukecarbis If we serialize the role then we can't query records by author role. For instance, we'd need to be able to do that for Stream Reports. Which actually makes think we should maybe even add a role column to the main Stream table.

@fjarrett
Copy link
Contributor Author

fjarrett commented Apr 6, 2014

@lukecarbis But yeah! Serializing the other user data into one meta key is a great idea!

@tylerhcarter
Copy link
Contributor

Just putting it out there, WordPress seems to believe in multiple roles. For most practical purposes, that doesn't affect us because usually no one is assigned more than one role and we could simply take the array WordPress gives us and grab the first entry. However, it is something to note for the future. When you grab user data by get_userdata, it could return multiple roles and we might want to account for that.

@westonruter
Copy link
Contributor

Yeah, that's an interesting feature. Currently, though, whenever you do assign multiple roles to a user, the additional roles will be lost the next time the user is updated due to the UI not supporting multiple roles. I seem to recall Nacin in a talk at WCSF that it was not recommended to use multiple roles.

@tylerhcarter
Copy link
Contributor

@westonruter In which case, we have nothing to worry about. I wonder if they'll start moving towards a single role model or they'll flesh out the multi-role to the UI.

@lukecarbis
Copy link
Contributor

Happy to take lead on this one. :)

@fjarrett
Copy link
Contributor Author

fjarrett commented Apr 8, 2014

@lukecarbis it's yours! /five

@lukecarbis
Copy link
Contributor

@fjarrett @powelski Just wanted to get some input on storing user_role_label as meta data.

In regards to translation, should we be storing a snapshot of the translated user role (user_role_label)? If my language is English, then I change to Italian, some of my Stream records will display the author role as administrator, and others as amministratore.

Or should we avoid storing the user_role_label? This way we would get the role label as we need it, via the wp_user_roles option. If my installation is English, then I change to Italian, all of my Stream records will display the author role as amministratore.

I tend to think the latter is the best option, because knowing what the label was when the action occurred isn't as important as knowing what the label means. This would mean that there is no need to store user_role_label as meta.

In the rare case that a role is deleted, we would still be able to use the role slug.

@fjarrett
Copy link
Contributor Author

fjarrett commented Apr 9, 2014

@lukecarbis I would actually prefer the former. Going back to the original point in this issue, the goal in mind is that Stream should be a true snapshot of history. In other words, not being dependent on anything currently loaded in WordPress, such as for its labels.

There are plugins that add new roles all the time, and those plugins could be deactivated, or the role labels themselves could be changed.

I don't think Stream should care if the language changes either. It shouldn't matter because its mission is just to show you what action happened at a particular time in history - a snapshot of the information that was captured in that moment. So if your language was Italian, then your records' labels are going to be in Italian.

The same could be said for the author Display Name. If your name was "Mr. Carbis" and you later changed it to "Luke Carbis", Stream should use whatever is currently in WP for the filter dropdown, and your user ID hasn't changed so it doesn't affect queries, but there should be different Display Names appearing in the Stream. Those that you performed as "Mr. Carbis" and those performed as "Luke Carbis".

The argument could be made that doing it any other way is actually making Stream's data historically inaccurate. Stream is supposed to be a reliable log of history.

So with this kind of perspective in mind, doesn't it make sense that we store the role label just as we store other things (like post title) as meta data?

@lukecarbis
Copy link
Contributor

I like your point. I can see a few problems arising from this approach (that I'm sure can be overcome, but are worth mentioning).

Suppose my Stream Records include three entries from the same user (who is having trouble deciding on a display name). In one record, the label is "fjarrett", in another "Mr. Jarrett", and finally the most recent is "Frankie". There could be some problematic usage cases here:

  1. I see the record authored by "Frankie", and decide I'd like to find other records by "Frankie", so I choose him in the Author Filter. In the current implementation of Stream, I would also see records authored by "fjarrett" and "Mr. Jarrett" because the filter is based on ID. I might think this is an error.
  2. I see the record authored by "Mr. Jarrett" and decide I'd like to find other records by "Mr. Jarrett". I try to find him in the Author Filter, but he's not there (Frankie is, but I don't know that there's a connection between the two).
  3. I am a Danish speaking Network Administrator and I'm browsing through the Stream records of my sites (and I exist in a future where Issue-65 Enable Stream on multisite / network installs #154 has been merged). I see one entry by "fjarrett" and I want to know what his user role is. Underneath his name is the English word "Editor" but that means nothing to me (or maybe it means something to me, but it's not "someone who edits").

I don't like to suggest problems without offering solutions.

Should we store things like user email and display name as meta? Resoundingly yes. If the user is deleted, then we need to display their details. However, even though "Frankie"'s display name was "fjarrett" when he edited the post, I think that the best and most relevant information to present to the user is his current display name (if it exists).

This removes a lot of confusion, and potentially solves some translation issues. I don't think that the author's display name being different at the time represents anything meaningful in the context of the fact that they updated a post. If I were to use Stream Undo to revert this change, I would be removing the updates to the post, not changing "Frankie"'s display name.

I think I've got that off my chest now, so if you still disagree I'm happy to approach things differently. I just think that the debate is important and really worth having

All this can be boiled down to a really simple choice: The situation is that a user changed his display name twice. What should the record summary for the first change be?

fjarrett changed his display name from fjarrett to Mr. Jarrett
or
Frankie changed his display name from fjarrett to Mr. Jarrett

@fjarrett
Copy link
Contributor Author

fjarrett commented Apr 9, 2014

@lukecarbis These are excellent use-cases of potential problems. Thank you for taking the time to clearly communicate each one.

  1. I love the fallback approach. Storing the user meta, but only using it if the User has been deleted. Could we go a step further and even add some helpfulness to the UI?

screen shot 2014-04-09 at 5 58 41 pm

  1. What about if the User's role changes, or no longer exists? Then what would we display as the role?

This would have impact on reporting for instance, because if I wanted to see a chart of Posts Activity by Author Role we don't want activity from "Frankie Jarrett" showing up under the Administrator data if he was actually an Editor when the activity was logged. /cc @Chacha any thoughts on this?

  1. Your fallback approach for roles would work for multilingual network admin issues too. With the edge-case caveat being that if the Role is deleted (for whatever reason) AND there is not a translation for the role available in WP core, then we would end up using the Role label from the Stream meta, which would be in the site's native language, right?

  2. Your last example of summary message isn't applicable here because summaries for user profile changes don't look like that. We would never say "his" or "her" because there is no gender meta (yet....haha). Also, we don't include which field was changed (yet), only that the profile was updated.

Frankie Jarrett's profile was updated

@lukecarbis
Copy link
Contributor

@fjarrett

  1. Yes! Adding 'Deleted User' to the UI is an excellent idea.
  2. We are still storing the author_role in the database (not as meta), so I think you could still find activity by administrators - that's not a problem. I would have three steps to getting a role label:
  • Get the user's role. If the user doesn't exist, use the role stored in the database.
  • Get the label for that role from get_option( 'wp_user_roles' ). If no label for that role exists, then use the label stored in the database.
  • If there's no label stored in the database, then use the role slug.

Potentially, we could avoid storing the role label in the database at all. In situations where the role has been deleted, we could just display the role slug instead (since this is already stored in the database).

  1. Yes - if a role has been deleted then the role label would most likely not be translatable. Maybe this is another reason to display a slug for deleted roles?
  2. This was more of a hypothetical.

@fjarrett
Copy link
Contributor Author

@lukecarbis I like the approach we've discussed here. I say we proceed with the fallback approach you've come up with 👍

@lukecarbis
Copy link
Contributor

@ThemeBoy I think we sorted this one out, but if you have any additional insight I'd love to hear them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants