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

Added ability to mark a card as done #4137

Merged
merged 8 commits into from Nov 8, 2023
Merged

Added ability to mark a card as done #4137

merged 8 commits into from Nov 8, 2023

Conversation

TehThanos
Copy link
Contributor

@TehThanos TehThanos commented Oct 18, 2022

Summary

This adds the ability to mark a card as done.
When the card is marked as done if it has a due date then it makes it un-editable, if it doesn't it simply displays "No due date"
Marking a card as done also hides it from the upcoming section.

TODO

  • Add Rest API documentation
  • Design changes requested form @nimishavijay
  • Remove l10n changes
  • lint issues
  • style lint issues
  • document headline number wrong here
  • missing cypress tests
  • Move from boolean column to datetime to store that exact timestamp when a card was marked as done
    • Change migration
    • Properly handle the datetime in the backend
    • Maybe indicate the done timestamp in the frontend on hover of the icon or in the details view next to the done label
    • Expose it through the caldav backend Added ability to mark a card as done #4137 (comment)
    • Double check visibility of icons in archived state
  • Find a proper layout for modal/sidebar for the done state and buttons

Follow up issues

  • Done by to store which user marked it (as suggested by @z0rgster)

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Documentation (manuals or wiki) has been updated or is not required

Screenshots

Current state (12th September)

Screenshot 2023-09-12 at 15 42 26

Screenshot 2023-09-12 at 15 42 32

Screenshot 2023-09-12 at 15 42 35

@TehThanos
Copy link
Contributor Author

TehThanos commented Oct 18, 2022

I ran the tests and the only ones failing are the ones in this file tests/integration/features/acl.feature.
The weird thing is they fail regardless of if I include my commits or not, is this expected or do I have a server configuration error?

@juliushaertl

This comment was marked as outdated.

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Very nice, really cool to see this pull request :) I've left a few comments inline, but it looks good to me in general.

Maybe you could post some screenshots of the frontend changes as well in order to get a review from our designers @nimishavijay and @jancborchardt :)

l10n/en_GB.js Outdated
@@ -65,7 +65,12 @@ OC.L10N.register(
"Description" : "Description",
"Formatting help" : "Formatting help",
"(group)" : "(group)",
"Done" : "Done",
Copy link
Member

Choose a reason for hiding this comment

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

Translations are handled through http://transifex.com/nextcloud/nextcloud/ so those changes would get overwritten by the nightly sync unless the translation is added there, but fine to keep them for the pr.

Copy link
Member

Choose a reason for hiding this comment

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

This one is still valid

lib/Migration/Version1000Date20200306161713.php Outdated Show resolved Hide resolved
lib/Service/CardService.php Outdated Show resolved Hide resolved
lib/Service/CardService.php Outdated Show resolved Hide resolved
src/components/cards/badges/Done.vue Outdated Show resolved Hide resolved
src/store/card.js Show resolved Hide resolved
lib/Controller/CardApiController.php Outdated Show resolved Hide resolved
lib/Controller/CardController.php Outdated Show resolved Hide resolved
@TehThanos

This comment was marked as outdated.

@TehThanos

This comment was marked as outdated.

@TehThanos

This comment was marked as outdated.

@TehThanos
Copy link
Contributor Author

I am not sure what's up with my dev environment but now when I try to run the integration tests I get APM: Locking APM for 300 seconds for reason: Cannot connect to the agent and while the do run, it takes forever. I don't have time today so I haven't run them but I'll take a look at it tomorrow, sorry.

@TehThanos
Copy link
Contributor Author

composer run psalm still returns ERROR: MoreSpecificImplementedParamType - lib/Migration/Version10900Date20221020074643.php:44:56 and I am not sure how to fix it since I also get UndefinedClass for SimpleMigrationStep on my system. If anyone can help me I would appreciate it. Please also take a closer look at commit 44fbb5a6

@nuokh

This comment was marked as outdated.

@nuokh nuokh mentioned this pull request Apr 25, 2023
@TehThanos

This comment was marked as resolved.

@raimund-schluessler
Copy link
Member

raimund-schluessler commented Apr 25, 2023

Since Deck offers an exposure of its cards via CalDAV, I think it would make sense to align with the RFC 5545. Specifically, the done/completed state of a todo is tracked by the COMPLETED property which stores the Date-Time the todo was completed at. See https://datatracker.ietf.org/doc/html/rfc5545#section-3.8.2.1.

So instead of storing a boolean value done in the database, I would propose to call the property completed and let it store a date with UTC time instead. That would better map to the properties of a todo defined in RFC 5545.

Edit:
I would then also adjust these lines

deck/lib/Db/Card.php

Lines 142 to 148 in bd9538d

$event->STATUS = $this->getArchived() ? "COMPLETED" : "NEEDS-ACTION";
if ($this->getArchived()) {
$date = new DateTime();
$date->setTimestamp($this->getLastModified());
$event->COMPLETED = $date;
//$event->add('PERCENT-COMPLETE', 100);
}

to correctly respect the COMPLETED property instead of the date the card was archived/last-modified.

@nimishavijay

This comment was marked as resolved.

@z0rgster

This comment was marked as outdated.

@luka-nextcloud

This comment was marked as duplicate.

@TehThanos

This comment was marked as outdated.

Copy link

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

Small linguistic suggestions in my review. (Really looking forward to this feature, thank you for putting in the work!)

docs/User_documentation_en.md Outdated Show resolved Hide resolved
docs/API.md Outdated Show resolved Hide resolved
docs/API.md Outdated Show resolved Hide resolved
@luka-nextcloud

This comment was marked as duplicate.

@raimund-schluessler

This comment was marked as duplicate.

@juliushaertl
Copy link
Member

I'll try to collect open topics now in the first post to make the conversation a bit cleaner.

@juliushaertl
Copy link
Member

Note you can ignore the one cypress failure, this is also an issue on the main branch

@juliushaertl
Copy link
Member

Thanks again for the nice PR. I did a rebase onto the latest main branch, i'd pick this up to adapt to storing the datetime instead of a boolean during the next days so we can get this in soonish. Hope that works for you :)

@TehThanos
Copy link
Contributor Author

Yes, I'm very sorry I haven't been more active.

@juliushaertl juliushaertl self-assigned this Oct 25, 2023
@juliushaertl
Copy link
Member

Moved to a datetime now and rebased again. Will put a checklist on what is still missing above

TehThanos and others added 8 commits November 8, 2023 12:59
Closes #534

Signed-off-by: Thanos Kamber <thanos.kamber@gmail.com>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl juliushaertl merged commit 6ea878e into nextcloud:main Nov 8, 2023
28 checks passed
@stefan-niedermann
Copy link
Member

So how does this feature eventually work?

  • The REST endpoints are the same as before?
  • The card properties do have an additional timestamp property from Done?
  • Is there an UI toggle for the Done property or a automatism when moving from / to the last list? Or both?
  • Are ETags updated properly when the flag changes?

@donty
Copy link

donty commented Nov 16, 2023

Seen the update for v 1.11.1 come through on our Nextcloud but I don't see the Done option in the UI anywhere logical - to me anyhow ;-) Looks like it was merged before the update so just wondering if I missed something?

@shoetten
Copy link
Contributor

shoetten commented Nov 16, 2023

Seen the update for v 1.11.1 come through on our Nextcloud but I don't see the Done option in the UI anywhere logical - to me anyhow ;-) Looks like it was merged before the update so just wondering if I missed something?

It was merged before, but not backported to the v1.11 / stable27 branch ;) See the release notes. It will ship with v1.12.0, probably with the next major of nextcloud, i guess.

@donty
Copy link

donty commented Nov 16, 2023

Thanks! So I did miss something ;-)

@sickstee
Copy link

sickstee commented Dec 9, 2023

Would it be possible to implement marking a card as done in the "reminders" app on IOS or any other to-do app that pulls infos via CalDav from the server? By now, marking a card as done in the "reminders" app doesn't pull the state to the deck app :octocat:

@stefan-niedermann
Copy link
Member

@TehThanos and @juliushaertl would you mind having a look at some open questions to be able to ship this feature also for the Deck Android app? 🙂

@TehThanos
Copy link
Contributor Author

The REST endpoints are the same as before?

Yes, with the addition of /cards/[id]/done and /cards/[id]/undone

The card properties do have an additional timestamp property from Done?

The done property is a datetime if the card isn't done it will be null

Is there an UI toggle for the Done property or a automatism when moving from / to the last list? Or both?

I'm not sure what exactly you mean, there is a button to mark the card as done or not done, I'm don't know about any automations.

Are ETags updated properly when the flag changes?

I believe so.

stefan-niedermann added a commit that referenced this pull request Jan 18, 2024
As stated in #534 (comment) updating the done property of a card via the REST API (without calling the /done and /undone endpoints explicitly) does only work "one way".

This commit allows setting null as new value thus allowing to mark cards as undone without an additional HTTP request but within a usual update request.

Refs: #534 #4137 c3b4ed6

Signed-off-by: Stefan Niedermann <info@niedermann.it>

Signed-off-by: Niedermann IT-Dienstleistungen <stefan-niedermann@users.noreply.github.com>
backportbot-nextcloud bot pushed a commit that referenced this pull request Jan 18, 2024
As stated in #534 (comment) updating the done property of a card via the REST API (without calling the /done and /undone endpoints explicitly) does only work "one way".

This commit allows setting null as new value thus allowing to mark cards as undone without an additional HTTP request but within a usual update request.

Refs: #534 #4137 c3b4ed6

Signed-off-by: Stefan Niedermann <info@niedermann.it>

Signed-off-by: Niedermann IT-Dienstleistungen <stefan-niedermann@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Mark card as done