Skip to content
This repository has been archived by the owner on Apr 27, 2023. It is now read-only.

Allfollowups #22

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

gmkumar2005
Copy link

PR for resolving -- Tracking the followups #21
Adds a new Dashboard tab

image

@lukaszlenart
Copy link
Member

I would rather call it Dashboard and do not restrict only to admins - all users should be able to view it.

@gmkumar2005
Copy link
Author

Renamed the allfollowups to Dashboard and will be visible to all users.
Not sure how to re-factor the UI layer.
Renaming the controllers and methods may not be sufficient.
The dashboard eventually would have additional functionality like.. statistics, search filters , graphs, etc..
All Followups service on server side could remain as is. Since it is a sub component of Dashboard
I need an advice on how to organize the ui layer.

@@ -9,9 +9,13 @@ trait FollowupFinder {
def findAllFollowupsByCommitForUser(userId: ObjectId): FollowupsByCommitListView

def findFollowupForUser(userId: ObjectId, followupId: ObjectId): Either[String, SingleFollowupView]

def findFollowupforAdmin(followupId: ObjectId): Either[String, SingleFollowupView]
Copy link
Member

Choose a reason for hiding this comment

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

this function should be renamed to findFollowup - we do not restrict to admins anymore, right?

@lukaszlenart
Copy link
Member

Not sure how to re-factor the UI layer.

I think the naming is ok - Dashboard can be treated as a virtual integration of several functions, ie. all followups, statistics, and so on. Let's start with something and see how it works.

@mostr
Copy link
Contributor

mostr commented Feb 9, 2015

I'm reviewing and playing with this changes and one thing I'm not sure about is whether it's ok to reveal all comments' content to all users. If it's just to find out if there are pending reviews (followups not answered or not "done") it should be enough to just display number of all followups (maybe grouped by user too, e.g. john has 3 followups). What do you think?

@gmkumar2005
Copy link
Author

@mostr Lets walk thru a scenario . Given I am scrum master assisting the team to complete their tasks. I like to look at all the followups for a) Understand if code reviews are happening 2) Evaluate if the reviewer is able to review based on project guide lines say.. coding standards 3) Identify critical remarks and ensure high priority items get needed attention.

@amorfis
Copy link
Member

amorfis commented Jun 8, 2015

I'm checking out the changes. What I found out is that on Dashboard I can see all the followups, and I can press "OK THX" under each. It doesn't work, as I can't mark as done a followup which was not sent to myself, but the button should also not be visible.

Besides, I agree with @mostr that showing all the followups to all the users might be not necessary. Maybe there should be number of followups shown, but clickable, and unfolding the full text?

Also, I think number of commits waiting to review for each user should also be on Dashboard. If it's purpose is tracking project development.

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

Successfully merging this pull request may close these issues.

4 participants