[WIP] Shared completion for Group Dailies

[MeanderingCode]

NOTE

this depends on #11389 now

---

This PR adds shared completion for Group Dailies as well as allows "uncompletion" for Group Dailies and Todos.

I am highlighting several places where I would like discussion on the feature or information on how Habitica works.

Fixes #10690

Changes

• "Single" Shared Completion Dailies are marked completed for all assigned users [whose "day" is the same as the completing user] when one assigned user completes
• Cron checks for completion of "Single" Shared Completion Dailies
• Master Daily (Single Completion) on Group Task Board is marked complete when one assigned user completes
• Dailies and Todos can be "uncompleted", updating/syncing that Task to assigned users and the Master Task
  • User cannot "uncomplete" a Single Shared Daily another assigned user completed
• Adds optional /yesterdaily segment to the end of the task scoring route which modifies history date to user's "yesterday"
• Completed Dailies due/not due filters work in Group Task Board list
• Fixes counting bug causing Shared Completion All Approval Required Tasks to complete when only one assigned user still needs approval and an approved user completes the task
• Add Mongo version key (__v) to omitAttrs in syncableAttrs for Tasks
• Add tests for Todos and Dailies shared completion syncing

WIP / Todo

• More tests
• History for Shared Completion All type tasks
• Uncomplete Group (Master) Dailies on someone's cron

Postponed for later

• Track Group Board Dailies completion based on user's "day", vs "first" user cron?
• Sync tasks to assigned users on master task save
• Master task streaks for dailies and habits
• Potentially: Reward Group with Gems based on Streaks?

---

UUID: 7d328dff-0f2f-4ecb-b4d4-dd323b1b676d

[MeanderingCode]

This is the main point of feature discussion.
I based my implimentation (complete other users' dailies) on cron: https://github.com/HabitRPG/habitica/blob/develop/website/server/libs/cron.js#L310

The implications of completing the task are that:

• The user who completes the Daily will score the task
• Other users will see the Daily is completed, will not score the task, will not lose their streak or take damage
• Other users will have their MP replenishment math include the done daily
• Other users "uncompleting" the daily has a bug in scoring (see [WIP] Shared completion for Group Dailies #11179 (comment) and following)

More discussion on how the feature should work may be found in #10690

[MeanderingCode]

I am not sure I am done with this feature, but wanted to put a draft up to start getting feedback.

[paglias]

@MeanderingCode thanks for the PR! I left a couple of comments about promises and callbacks but regarding the PR feature I'm going to assign it to @SabreCat since he's working on Group Plans

[MeanderingCode]

In more reworking and testing, I have discovered a bug. If UserA completes a task and UserB realizes the task is not done and "uncompletes" it, UserA's streak remains unchanged, but UserB's streak decriments.

I need to look into how to avoid that. My first thought is to look into the task history property of the various copies of the task. This can only apply to Dailies, as single completion shared Todos are deleted from the other users' tasks.

[MeanderingCode]

Having read more of the scoring, it looks like there are two approaches; one is messy and the other requires some careful work in https://github.com/HabitRPG/habitica/blob/develop/website/common/script/ops/scoreTask.js#L187

In the messy approach, groupTasks.js could:

1. If a task is completed, copy the user's last history entry, add their userId, and append it to the master task's history
2. If a task is "uncompleted", read the userId from the last history entry in the master task, remove the history entry, and if that userId is not the user who "uncompleted" the task: score down the user who completed it and score up the user who "uncompleted" it.

This is messy b/c it shows down and up scores for the "correcting" user and may not stay up to date if code changes in other areas.

It would be far better to adjust scoreTask() so that it had logic to handle group shared tasks. This also opens the door for using scoring, streaks, etc on group shared tasks.

Alternatively, an earlier approach I had thought of could be taken: Instead of completing the dailies on other assignedUsers' lists, simply set a startDate of tomorrow, making them grey (not due). It doesn't tell the user the task has been done (mobile apps can't see the group task board), but it does tell them they don't have to do it.

In looking through code for this, it appears challenges have a lot of the features I'm looking for (though not all), and I am very curious to know more of the discussion or design thinking going into groups. I have heard of some redesign talk, but I have not been able to find anything useful in trello or github about it.

@SabreCat , perhaps you have insight or can point me at where such discussion may be happening?

[MeanderingCode]

There appear to be many things at play here.

• Modifying the task.startDate could have adverse effects on shared scheduling of everyX days dailies
• Modifying the task.startDate requires integration with syncTask
• Marking other users' task.completed has implications on their MP (actually okay, i think), but has side effects I'm looking into WRT "uncompleting" tasks
• Perhaps the best option is to integrate checking for group completion in the shouldDo() function. Still investigating: https://github.com/HabitRPG/habitica/blob/develop/website/common/script/cron.js#L98

I am sorry to be having a one-sided design discussion here. I continue posting as I'm learning in case someone wants to weigh in or has knowledge that may point to the right direction.

[paglias]

Hello again @MeanderingCode ! I just wanted to let you know that we've not forgot about this PR and we're going to discuss it in a couple of days and let you know :)

[SabreCat]

Hi @MeanderingCode--thanks for diving into this! A few comments and questions for you...

1. Gem awards are out of scope for addressing this ticket, and not something we'd want anyone starting on at this time. It's a clever idea, though, and something I'd recommend including in an email to admin@habitica.com as Group Plans feedback!
2. Similarly, if the "uncomplete" flows (or streak syncing, etc.) are proving to be a hassle, I'd recommend implementing only shared completion of Dailies in this PR. We can get that feature functional and polished before moving on to another workflow!
3. Do I understand correctly that so far, this only implements the "single" completion condition for Dailies, not "all assigned"?
4. Re how to mark individual users' Dailies as done after the master task is done via single-completion, I would definitely prefer setting the completed status rather than adjusting the Start Date. The Start Date of a task can be used in meaningful ways by Group Plan managers/leaders to note when a daily responsibility begins, information that would be lost if overwritten when the task is done on a specific day.

[MeanderingCode]

Hi @SabreCat. Thanks for taking a look! My family is very excited to have this feature, so I'd love to move this forward.

1. Gem awards are out of scope for addressing this ticket, and not something we'd want anyone starting on at this time. It's a clever idea, though, and something I'd recommend including in an email to [admin@habitica.com](mailto:admin@habitica.com) as Group Plans feedback!

I agree it's an enhancement that's beyond this PR. I'll email about it!

2. Similarly, if the "uncomplete" flows (or streak syncing, etc.) are proving to be a hassle, I'd recommend implementing only shared completion of Dailies in this PR. We can get that feature functional and polished before moving on to another workflow!

I will make the cut where it makes sense. I do not want to disallow "uncompleting", because it could prove problematic if parties are relying on the status of tasks which need to get done. More below.

3. Do I understand correctly that so far, this only implements the "single" completion condition for Dailies, not "all assigned"?

No, this works for both "single" and "all" shared completion types, for both completing and "uncompleting".

4. Re how to mark individual users' Dailies as done after the master task is done via single-completion, I would definitely prefer setting the `completed` status rather than adjusting the Start Date. The Start Date of a task can be used in meaningful ways by Group Plan managers/leaders to note when a daily responsibility begins, information that would be lost if overwritten when the task is done on a specific day.

That echoes my own intuition and thinking, without specific knowledge of how it is used.

With regards to the bug where UserB "uncompletes" a task UserA erroneously completed and the scoring/streak discrepancies, I think this can fit into an enhancement workflow around group task scoring. If UserB "uncompletes" a task UserA completed, they should be able to reverse the damage by completing the task. They won't get credit for doing it, but neither will they get damage. Leaving "uncomplete" in allows for the user who made the accidental click to rectify the mistake.

With enhancements [later] for a group scoring feature, the necessary enhancements will make it easier to do things like changing the shouldDo() and setNextDue() logic.

If all that is agreeable, I will finalize this PR and mark it ready for review!

[MeanderingCode]

Addressed all but two linting concerns:

• Indentation is as it should be, as far as I think. I could move the .then(task => { to a new line at the same indentation as the line it is currently on, which may address the linter's concerns, though I find mixed usage in the code. Thoughts?
• What about the await in the for loop? It is done to address the ParallelSaveErrors from mongoose. Please comment on above code comment thread on this topic.

[MeanderingCode]

There is, indeed, a bug with approval. I will retitle this PR with "WIP". Please refrain from merging.

[MeanderingCode]

Yes, the intention is that you cannot uncomplete a daily another assigned user completed.

[paglias]

Ok then what's the point of pushing something in the task history at the line before if it'll never be saved due to the error? Il Lun 10 Giu 2019, 13:25 Sean Leonard <notifications@github.com> ha scritto:

…

Yes, the intention is that you cannot uncomplete a daily another assigned user completed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11179?email_source=notifications&email_token=AAEQAV4WOV7NBVXUOFD22LDPZY2Y7A5CNFSM4HN3IP52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXJT4MQ#issuecomment-500383282>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEQAVZH5TGJ4L7EJ3PAECLPZY2Y7ANCNFSM4HN3IP5Q> .

[MeanderingCode]

Good point. I'll remove, as well as clean up comments slightly if the approach seems valid.

[MeanderingCode]

@paglias

Ok then what's the point of pushing something in the task history at the line before if it'll never be saved due to the error?

I had done that, as well as moved the check before the task.completed = false, because the task remained "unchecked" after the throw, and I didn't want to risk a modified client-side history to be inadvertently saved to the server in some later send. Is that a risk? What do you think?

---

Aside: I wish GH email reply comments properly nested under the thread you started.

[MeanderingCode]

The test failures were mostly due to needing to change the cron tests to async/await to match my change to the cron function. Another test (one of mine) needed to call a function for more depth regarding task completion, and one potentially good catch led to a defensive logic change for potential edge cases.

I think this is ready for review, @paglias and @SabreCat.

[SabreCat]

FYI we're putting this through some QA with the help of volunteer testers from the community. @MeanderingCode you've got it tagged as WIP still--is there more you plan to implement yet?

[MeanderingCode]

Hey @SabreCat. Glad to hear it!

As the description states, I need to add some more tests, add history to "All" type tasks (simple), and decide when and how to cron the group dailies. If I don't cron them, the group task list dailies stay completed (though individuals' linked tasks cron with them).

[MeanderingCode]

Hi @SabreCat. Any updates on how testing is going? Any bugs?

I will try to write the tests soon, and am happy to push the additional things to another PR if it makes this easier.

[SabreCat]

I checked in with the crew again today, and while no bugs have been reported, it doesn't sound like intensive testing's been done--the trials of being a small team with no dedicated QA staff, tch. We're going to set aside one of our regular meeting times to get together and try to break things in the next few days, though!

[SabreCat]

Hi @MeanderingCode -- I committed a fix to one issue we ran across, but other than that the functionality has been working well! It might make sense to wrap up the tests here and push any further nice-to-have functionality to another PR after we merge.

[paglias]

Hi @MeanderingCode, we were wondering if you plan to add the missing tests or if you need help with them! If you can't no worries and we'll add them ourselves :) thanks and let us know

[MeanderingCode]

@paglias @SabreCat I had planned on adding them, but I have had no time for it in a long while.

When I get home, I can copy my notes on what tests I was planning to make it easier for someone to know what to cover. If no one picks it up, I may find time in the next couple weeks.

[paglias]

@MeanderingCode no worries, and yeah if you can post here the list of what you planned to cover it would be very useful!

[SabreCat]

@MeanderingCode @paglias Took a stab at adding the missing group daily uncomplete at cron in 14d68e0 . Pretty first-drafty, thoughts?

@MeanderingCode Getting anywhere with the checklist of tests to add?

Also after merging latest develop a relevant test is failing? 🤔 Odd... Oh, guess that was flukey.

[paglias]

left some comments!

[paglias]

I'm closing this in favor of #11390 (needed because of a change in the order of PRs that will be merged)