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

No Error Handling / check for missing attributes in Notes #218

Closed
liwde opened this issue Jan 17, 2019 · 8 comments
Closed

No Error Handling / check for missing attributes in Notes #218

liwde opened this issue Jan 17, 2019 · 8 comments
Assignees
Labels
Type: Bug Type: UX concerning user interface and in a broader sense user experience
Milestone

Comments

@liwde
Copy link
Contributor

liwde commented Jan 17, 2019

When opening the Notes with "useTemporaryDatabase": false in the config.default.json, a pretty non-descriptive error-message is shown in a red Message-Toast: "missing(undefined)"

In the console, a few more messages appear: The DB could not find an entry to be loaded ({"status":404,"name":"not_found","message":"missing","reason":"missing"}) and a later error where hasPhoto() is called on undefined. It seems like the Child related to a Note is not found resulting in the error.

In fact, Note:1536258354699 references a child Child:new that does not exist. So to fix this now, we could just delete the Note. However, we need to also think about fixing the root cause:

1. No Error Handling

child-block.component.ts has no error handling:

ngOnInit() {
    if (this.entityId !== undefined) {
      this.entityMapper.load(Child, this.entityId).then(child => {
        this.entity = child;
      });
    }
  }

I'm not sure if we could let an error here bubble up to the containing notes-manager.component.html to somehow remove the child from the Note, or whether a simple in-place message "Child deleted" would be better.

However, this relates to...

2. Foreign Key Handling

I don't know what happened to Child:new, but it must have been deleted somehow. Today, this is possible from our UI, which directly uses the EntityMapperService to delete the Entity. However, this does not delete foreign keys (e.g. in Notes) or even Entities (e.g. the ChildSchoolRelationship). Are there already ideas on how to handle this? Otherwise, we should think about a concept on how to handle cases like this.

@liwde liwde added Type: Bug Type: Refactoring / Technical Enh. Technical Enhancement without changes for the user discuss Type: UX concerning user interface and in a broader sense user experience labels Jan 17, 2019
@sleidig sleidig added this to the [v2.6] milestone Jan 17, 2019
@sleidig
Copy link
Member

sleidig commented Jan 17, 2019

Somewhat related: The current live system was broken due to a record with note.date = null, causing the NotesManager to remain blank without showing any notes. Console logged the error

Uncaught (in promise): TypeError: Cannot read property 'getTime' of null

We do need better error checking. Maybe this should best go into the Entity sub-classes like Note itself (should we define getters?) and fill it with a reasonable default value or some kind of "Null Object". Then we don't have to repeat ourselves with error checking conditions all over the place.

@sleidig sleidig changed the title Error in Notes No Error Handling / check for missing attributes in Notes Jan 17, 2019
@sleidig
Copy link
Member

sleidig commented Jan 17, 2019

I would suggest to limit this issue to the error handling / attribute checking.

@liwde, do you want to create a separate issue for the Foreign Key Handling / cascading delete / ... question? That sounds like a big but important topic.

@liwde
Copy link
Contributor Author

liwde commented Jan 23, 2019

@sleidig, I completely agree, these are two different issues. I opened #220 for the long-term solution, so we can focus on handling the immediate effect and error-handling for Notes here.

@Lorsbyjm
Copy link
Contributor

Lorsbyjm commented Feb 4, 2019

So the solution for this issues is to define getters for every attribute to check if a value is set or return a null object. Then we set the attributes to private and have to change all access to each of them. Am I right @liwde @sleidig ?

@sleidig
Copy link
Member

sleidig commented Feb 4, 2019

Yes, I think getters/setters would be the proper way to go. Should be tested with the CouchDB setup to make sure the getter/setter/private stuff is not creating problems when writing the object to the database

@liwde
Copy link
Contributor Author

liwde commented Feb 4, 2019

This is not about getters and setters, but the object itself being undefined. Instead, we must create a .catch(error=>...) to display a reasonable error and handle the child being undefined at the UI.

@Lorsbyjm
Copy link
Contributor

Lorsbyjm commented Feb 4, 2019

Okay, so this ist still facing two problems. The first one, lukas mentions, about missing error handling and deletion of Objects.
And the second one, getters ans setters for objects.
I think the firts one in now more important.

@liwde liwde self-assigned this Feb 7, 2019
@sleidig
Copy link
Member

sleidig commented Jun 17, 2019

As far as I understand it, PR #225 fixes the discussed points. @liwde, @Lorsbyjm please reopen this or create a new ticket if I am missing something.

@sleidig sleidig closed this as completed Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Type: UX concerning user interface and in a broader sense user experience
Projects
None yet
Development

No branches or pull requests

3 participants