search

LearnersGuild / echo

learning management system
MIT License
3 stars 31 forks source link

Players can submit retro for any project independently of cycle state #629

Closed jeffreywescott closed 7 years ago

jeffreywescott commented 7 years ago

Critical Strategic Goals

Benefits

What are the benefits of this change, and whom do they impact?

Description

New behavior for the /retro command:

User issues /retro command (w/o project name)

  1. Searches for any open projects* the player is a member of.
  2. If none found, displays a message that there are no open projects.
  3. If only one found, automatically loads the retro survey for that project.
  4. If multiple found, display a list of links to /retro/:projectName. Load retro survey for project when clicked.

User issues /retro #:projectName command (w/ project name)

  1. If the player is not a member of the project, display error message.
  2. If the project is not open*, display a message.
  3. Else, automatically loads retro survey.

*For the purpose of retros, open projects will now be defined as any project in a cycle with state REFLECTION or later for which all members have not yet submitted their retro.

Original Description:

Description

Describe the change, and provide any needed context.

Players can /retro #project at any time to submit a retro for any project they have been on (unless they've already submitted a retro for that project).

Logic changes

Players can submit a retro for any of their projects that have been marked complete

With this, there is no need for a cycle to have a REFLECTION state.

  • Remove REFLECTION state from cycle model
  • Change /cycle reflect command so that it just marks all cycle projects complete.
  • Allow players to submit retro surveys for any completed project that they have been on (via the /retro #project command)

UI changes

/retro

/retro will load survey for the player's most recent completed project, if they have only one. Otherwise, will let them know that they have to choose one of their projects (no change from current UI/UX)

/retro

When submitting a retro for a particular project...

If the player has not already submitted a retro for that project: -> open the retro survey for that project

If they player has already submitted a retro for that project: -> show the following screen and error message

[view message:] The survey could not be loaded.

[error message:] Error: survey for project #<project-id> already submitted. To request an amendment to your response data, submit a new issue here: https://github.com/LearnersGuild/los/projects/1
jeffreywescott commented 7 years ago

Comments from issue on the design board:

@tannerwelsh commented on Fri Nov 18 2016

Logic + UI changes added. Ready for review @LearnersGuild/software

@jeffreywescott commented on Fri Nov 18 2016

So is there a hidden implication here that stats will be (re-)calculated after each new retro submission? If so, I'd recommend that we prioritize this after we have a pull-based stats UI (https://github.com/LearnersGuild/game-prototype/issues/109) to avoid confusion.

@tannerwelsh commented on Mon Nov 21 2016

You're right, there is a hidden implication @jeffreywescott. Now unhidden: #145

Prioritization is up to you, but frankly I think this can and should be done before #109. The confusion it may cause is less than the benefit we'd get from fewer arbitrary locks on data collection. Also, people are already confused by how stat calculation works :/

@jeffreywescott commented on Mon Nov 21 2016

So ... will we still only send one DM w/ stats, or a new DM every time the stats are re-calculated?

@shereefb commented on Mon Nov 21 2016

Not sure I understand what you're saying jeffrey. We're talking about retro not review. Stats are calculated one time once all team members have submitted their retros. That's when the DM goes out.

@jeffreywescott commented on Mon Nov 21 2016

Good point, @shereefb -- sorry -- I was confused, indeed.

@bundacia commented on Mon Nov 28 2016

I like this change. Two pieces of feedback:

1. Keep the Reflection State

I could easily see people accidentally submitting reviews for the current project on Mondays when they intended to submit one for the previous week since the forms will look identical except for the name at the top.

I'm wondering if we should keep the REFLECTION state (at least for a while). And have it work this way:

  1. Before a cycle enters REFLECTION you can't submit reviews for a project in that cycle.
  2. When the cycle is not yet in a REFLECTION state, typing /retro pulls up the review survey for your most recent past project. That way when people type retro on the first day of the cycle the systems acts as they would expect: pulling up the review for the project they just completed, not the one they just started.

We could implement this my adding a completed flag to projects and just setting that flag for all projects in a cycle when the cycle enters REFLECTION. Then we can default the /retro command to act on your "most recent completed project". That logic would continue to work as we decouple projects from cycles more; we could just provide a way for teams to mark their project completed manually. At that point the REFLECTION state could go away altogether. I suppose we could move to this model now, but it just adds work for all teams right now since currently all projects get completed at the same time.

2. What about /review?

Does this change affect how /review works at all? Should it?

@tannerwelsh commented on Mon Nov 28 2016

@bundacia thanks for catching that. What if we removed the default retro/review entirely? I.e. we make the #project-id parameter for both commands required, so people have to specify the project?

@bundacia commented on Mon Nov 28 2016

@tannerwelsh: I'm not sure that would solve all our problems. We'd still be allowing people to complete a retro for a project that was not complete. Since the surveys would have to be created "on demand" (instead of at /cycle reflect like they are now). You could run into strange cases like this:

  1. Someone accidentally issues a /retro #my-brand-new-project command on the first day of the cycle. Even if they realize their mistake and don't complete the survey, it's still been generated in the system for that project.
  2. Players get added to removed from that team
  3. Now the players the retro questions are asking about are out of sync with the actual players on the project.

So the even though the "just make the project name a required arg" fix would allow us to remove logic around finding the right project, which would simplify the system, we'd still have to add a bit of complexity in other areas to mitigate cases like the one above.

@jeffreywescott commented on Wed Nov 30 2016

@bundacia, it seems like (if we remove the REFLECTION state), when a project team changes composition (team members are added or removed), the system would have to delete any retrospectives that were submitted with the previous team composition, because they'd no longer be valid, right? Especially since people cannot change their retro answers once they've submitted them.

@jeffreywescott commented on Wed Nov 30 2016

Moving back to Review -- this isn't fully baked.

@bundacia commented on Thu Dec 01 2016

@jeffreywescott, yeah we'd have to do something like that, which seems a bit complex. I'd rather avoid jumping through all those hoops if we don't need to.

Reading over my suggestion again, I'm realizing we don't actually need the REFLECTION state at all. We can just change the /cycle reflect command to mark all of the projects complete. It's not really a state, just an event. Maybe we'd rename the command in that case. It would then be trivial in the future to add commands to mark individual projects complete.

@jeffreywescott commented on Thu Dec 01 2016

Nice thinking, @bundacia -- so do you think this is now RFI? I'll move it back over if so.

@bundacia commented on Thu Dec 01 2016

@jeffreywescott, the description would still need to up updated first I think. The Logic Changes section should read:

#### Logic changes

Players can submit a retro for any of their projects that have been marked complete

With this, there is no need for a cycle to have a `REFLECTION` state.

- Remove `REFLECTION` state from cycle model
- Change `/cycle reflect` command so that it just markes all cycle projects complete.
- Allow players to submit retro surveys for any completed project that they have been on (via the `/retro #project` command)

We also need to decide weather to continue to support /retro without an explicit project name, and update the description if not.

@shereefb commented on Fri Dec 02 2016

It seems to me that cycles should no longer haves states, and projects should track their states.

Cycles should be aware of their project lists, but projects should not care or know anything about their cycles.

Agree with @bundacia that cycle commands simply trigger their projects into a different state. So /cycle reflect switches all its projects to REFLECTION state, not COMPLETE. Once all retros and enough reviews are submitted for a project it goes into COMPLETE state and no more retros are allowed there.

We're going to need a REFLECTION state for projects to differentiate between projects that have a complete set of retros/reviews. For example, projects in REFLECTION state can nag/remind team members to get their retros in.

/retro command should not infer the project, and project id should be passed to it.

@bundacia commented on Fri Dec 02 2016

We're going to need a REFLECTION state for projects to differentiate between projects that have a complete set of retros/reviews. For example, projects in REFLECTION state can nag/remind team members to get their retros in.

I could definitely see us needing this in the future, but I'd rather create this extra reflection state for projects once we have a concrete use case for it. Not sure we need it now. Either way that's more of an implementation decision. Either way would support the use case for this issue, allowing players to submit retros for any project independent of cycle state.

@jeffreywescott commented on Fri Dec 02 2016

Okay, thanks, all. I've updated it as @bundacia suggested. We'll definitely want to do more work around decoupling cycle state from project state in the future, but that's definitely beyond the scope of this particular issue. Moving to RFI, and then eventually implementation.

heyheyjp commented 7 years ago

@tannerwelsh @shereefb @jeffreywescott @bundacia:

hey crew. some quick feedback/checking-in now that i'm picking this up for implementation:

nitpick numero uno seems unnecessary to use a temporary complete flag on the project when we know we're just gonna have to migrate that data again for a more flexible project state management solution. from my software engineering role...i'd really rather not.

nitpick numero dos i think we can do better than only loading the most recently completed project if a player doesn't provide a project name for the /retro command for very little additional cost.

proposal: backend changes

  1. introduce a complete project lifecycle. project states would be: PRACTICE, REFLECTION, COMPLETE (alt: CLOSED).

  2. do away with cycle state. now...not later. they become basically just a counter. cycle commands would then have the following effects:

    • /cycle init: creates a brand new cycle (ticks the counter) for the chapter and creates voting pools. every player not currently a member of a project in PRACTICE state is assigned to a pool and allowed to vote.
    • /cycle launch: forms new projects in the chapter based on active votes. projects no longer have a cycleId. their initial state is set to PRACTICE.
    • /cycle reflect: finds all projects in the chapter that are in PRACTICE state and changes their state to REFLECTION. generates surveys for each of these projects. as each team completes their retros and receives enough reviews, its stats are calculated and the projects are individually moved into COMPLETE state.

proposal: ui changes

  1. /retro: if for some reason you are on multiple "open" projects (either you're a pro player/PM or you've been assigned to a project but haven't yet completed your retro for a previous project), open the flex panel and display a list of your projects pending retro completion. this would be a huge gain in usability and would cost practically nothing. issuing this command when you have just one pending project retro, of course, loads that project's survey, and issuing it when you have no pending retros gives you a friendly "you've submitted all of your retros" kind of message.

thoughts?

tannerwelsh commented 7 years ago

👍 to the UI changes. no input on implementation.

do you need a mockup for the list view? for expediency, i'd say just use a default <ul> with project names.

jeffreywescott commented 7 years ago

@prattsj what's the cost factor of expanding this issue as you propose? The UI bit seems pretty straight forward, I agree. The other stuff seems potentially more expensive. Is it 2x? 5x? Just a rough idea would be helpful.

Also, @shereefb should chime-in as moderator as to whether that approach for the /cycle commands is what is wanted / needed.

Also, if we go this route, it seems like there are 2 separate issues here; one around /cycle, and one around /retro, perhaps part of the same epic.

heyheyjp commented 7 years ago

@tannerwelsh: sounds good. if we go forward with it, that'll be the plan. 👍🏾

@jeffreywescott: hard to say without looking at the existing related code, but my super rough estimate would be that it's probably somewhere around 3-4x all told. my thinking is that this additional cost would be made up for primarily by an elimination of the cost of doing rework already planned for this area of the system (the code, the testing, the review, the migration).

HOWEVER...the more i think about this, the more apparent it is that we don't have to change anything about cycle or project state management (insofar as it's modeled or persisted in the db) in order to deliver on the original objective of this issue (allowing folks to submit retros anytime after the project's reflection period begins and before they've submitted it the first time). we could just change the logic that loads the retro survey to ignore the state of the currently active cycle and present the user with the option to submit so long as they haven't submitted yet. ideally, this would also include the addition of ui to list all such projects if there are multiple pending the user's retro. similarly, the logic that handles responses would accept them so long as the user hasn't submitted them before. that change is less risky than every other proposal made here so far, and my rough estimate of cost is that it'd be around the same as the cost for the changes currently spec'd in the description.

jeffreywescott commented 7 years ago

In that case, @prattsj, let's go with your second suggestion, and defer the full deprecation of "cycle state" until another time. I really want us to keep moving things forward, and part of me would like to just have a "data model reset" epic where we tackle all of those things at once, hopefully early in Q1.

bundacia commented 7 years ago

The one issue with @prattsj's second suggestion is that it allows people to submit a review too early, which could lead to accidental submissions of the survey for the "current" project when the intent was to submit a survey for a previous project. This could be mitigated by having a clear UI I guess.

jeffreywescott commented 7 years ago

Yeah, if we make it so that /review pulls-up a list of projects, perhaps with teammate handles / names next to it, I think it will be okay.

heyheyjp commented 7 years ago

@bundacia: there's nothing about my 2nd solution that implies this should be possible. The idea is, query for any project in a cycle that has a state of REFLECTION or later and for which the user hasn't yet submitted a retro. Viola! :-)

heyheyjp commented 7 years ago

Description updated.