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

List PaperTrail versions per event #628

Merged
merged 6 commits into from Nov 6, 2019
Merged

Conversation

erdgeist
Copy link
Contributor

@erdgeist erdgeist commented Nov 3, 2019

This PR adds a "history" tab to the events view, listing all versions of the event and associated events.

It also cleans up the tabs on event views, so I didn't have to manually add the history tab to each of the others.

A review would be quite appreciated.

@erdgeist erdgeist changed the title List PaperTrail versions per conference List PaperTrail versions per event Nov 3, 2019
@erdgeist erdgeist closed this Nov 3, 2019
@erdgeist erdgeist reopened this Nov 3, 2019
@elad-eyal
Copy link
Collaborator

Some comments after reading the code (didn't try it yet)

  • Did you forget to include history.html.haml ?
  • I think the history view should exclude the review and feedback models (i.e. EventRating, EverentReviewAverage, ... ) for two reasons. (1) I think they will spam the list (2) it's a way to circumvent pundit authorization to see this data.
  • I propose to move the tab order (from left to right) to be: Event details, people, history, Rating, Feedback
  • Related bug you might want to fix while at it: Adding/removing/changing classifier for an event is not stored in papertrail.

@erdgeist
Copy link
Contributor Author

erdgeist commented Nov 3, 2019

  • Added the template.
  • As the history is meant for coordinator/organizer only, anyway, I think it should be limited to those users in the first place. Leaving the spamming issue. I don't consider ratings spam. This is exactly the kind of changes I'd expect in this place. So basically we only need to exclude feedback. That could be handled by an extra :verbose parameter.
  • Changed the tab order and, you're right, looks more natural
  • Will look into adding PaperTrail to classifiers in another PR

@elad-eyal
Copy link
Collaborator

@erdgeist I would think you'd want reviewers to see the history of the events they're reviewing, but not other events. I don't think it's important to track old review scores (on the contrary maybe). Anyway I'm not sure the reviews are integrated to paper-trail completely anyway.

@erdgeist
Copy link
Contributor Author

erdgeist commented Nov 5, 2019

I see no easy way to exclude Ratings and Feedback. Updates types are saved as yaml key/value pairs deep in the PaperTrail version object with no efficient way to filter for them.

That being said, I actually didn't find it distracting seeing these verbose updates in a live conference, as they do happen at very specific times in an event's life cycle, making it actually pleasing to follow it.

@erdgeist erdgeist merged commit 0e0352d into master Nov 6, 2019
@elad-eyal elad-eyal mentioned this pull request Nov 6, 2019
@erdgeist erdgeist deleted the erdgeist-event-history branch November 9, 2019 14:46
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.

None yet

2 participants