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

Implemented TaskCategory entity #355 #386

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

Conversation

kutakmir
Copy link

@kutakmir kutakmir commented Mar 16, 2020

Addresses issue #355.

A new TaskCategory entity was introduced. This feature helps to organize OCKTasks in categories.
This way the user can categorize the tasks by a medical condition, by the type of action, or in any other way they want.

Database modifications

New entities

TaskCategory
...
- title
- asset (image name)
- notes array (contains the list of textual notes created by the practitioners or the patient)

New relationships

TaskCategory -> CarePlan
CarePlan ->> TaskCategory

TaskCategory ->> Task
Task -> TaskCategory
-> has one
->> ... has many

Considerations

  1. Shouldn't we add an "Uncategorized" TaskCategory that will always be there and all the Tasks that are not associated with any TaskCategory will be displayed when this "Uncategorized" TaskCategory is selected?
    ...
    Currently, the taskCategory property is optional. To implement the "Uncategorized" TaskCategory would mean adding a special logic for this use-case. A more generic solution is to solve this issue upon task creation. If you create a new task, you'll choose a category. In no category is selected, there will be the default "Uncategorized" TaskCategory. But this logic is something to be decided by the end-user (the programmer of a particular app). Some apps might not want to categorize the tasks, so the default category would render no benefit.

  2. Nesting of the categories
    ...
    It's a bit tricky because Swift structs cannot be nested directly (to prevent infinite loop upon initialization). The alternative is to use IDs. Is this something that is desirable? What is the best way to achieve nesting?

  3. Gravirawson's suggestion [Proposal] Category Card, OCKTaskCategory, organization of OCKTasks #355 (comment): create an OCKCategory instead of OCKTaskCategory for CarePlans and other use-cases.
    ...
    What would be the use-case for the care plan category? Could be the same category reused in multiple different entities?

  4. Should the TaskCategory to Task relationship be many-to-many instead?
    ...
    This would provide more flexibility. Right now, I only have one task category per task for simplicity's sake. The many-to-many would enable to filter tasks by different sets of categories. This is especially useful for the implementation of the "All tasks" category (that we can include in the list of categories).

TaskCategory ->> Task
Task ->> TaskCategory
  1. What is the best way to integrate this new functionality in terms of UI?
    ...
    Really an open question for debate. I created a very basic category selection view controller that in turn presents the daily list of tasks filtered by a task category. Should the filtering happen on the main screen? Or would a completely different UI be more appropriate? The original idea was to create the same drill-down experience as in the Apple Health app.

  2. Should we keep the notes array in the OCKTaskCategory?

@erik-apple
Copy link
Collaborator

Thanks for the PR! We'll begin going over it and leaving feedback soon!

Copy link
Collaborator

@erik-apple erik-apple left a comment

Choose a reason for hiding this comment

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

I've only just given this PR a first pass. I still have a lot to go over, but here are a couple items for you to work on in the mean time!

Thanks again for putting so much time and effort into this PR!

@@ -85,6 +85,6 @@ class TestSimpleContactViewSynchronizer: XCTestCase {
viewSynchronizer.updateView(view, context: .init(viewModel: nil, oldViewModel: nil, animated: false))
XCTAssertNil(view.headerView.titleLabel.text)
XCTAssertNil(view.headerView.detailLabel.text)
XCTAssertEqual(view.headerView.iconImageView?.image, UIImage(systemName: "person.crop.circle"))
XCTAssertEqual(view.headerView.iconImageView?.image, UIImage(systemName: "questionmark.circle.fill"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we may want to keep the default image as "person.crop.circle" for the contact views.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, and we are using that. I introduced the questionmark icon there so that we can use it as a universal placeholder.

If you look at OCKHeaderView+Updatable, you'll see the updated logic. We are using OCKHeaderView for various purposes - Events, Tasks, Contacts, and TaskCategories. If the clear out and not configured state was using a person placeholder, it would look strange in the TaskCategory list.

The Contact list will not be impacted by this change. The difference is in the configuration. If you configure the OCKHeaderView with a OCKAnyContact that doesn't have any asset - the image will be rendered as a person placeholder. In case of the OCKAnyTaskCategory, a question mark image will be used.

Let me know if this is desirable logic or what other solution would you suggest.

CareKitStore/CareKitStore/CoreData/OCKCDTaskCategory.swift Outdated Show resolved Hide resolved
@kutakmir
Copy link
Author

@erik-apple for some reason the CI is giving me a failure in some of the unit tests, although they pass when I run them locally. Are you able to reproduce?

✗ testTaskQueryOnPastDateReturnsPastVersionOfATask, XCTAssertTrue failed - Expected to get 1 task, but got 0
✗ testTaskQueryOnPastDateReturnsPastVersionOfATask, XCTAssertTrue failed

@erik-apple
Copy link
Collaborator

Yes, that’s a known issue. We’ve noticed that it happens at a certain time of day, and only on Travis’s CI servers. We are unable to reproduce it locally as well.

If you wait a couple hours and rerun the tests they will pass. Don’t worry about it for now though, we’ll ensure they pass before we merge.

@kutakmir
Copy link
Author

@erik-apple Hi Erik, it's been awhile, would you mind giving me some guidance so that I can make all required alterations to get this PR merged? Thanks in advance!

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