steroid-team / app

Helping you organise your day with ease and privacy.
1 stars 2 forks source link

Feature/97 database persistency #151

Closed sydneyhauke closed 3 years ago

sydneyhauke commented 3 years ago

Persists tasks, notes and todolists in remote database.

The database interface needed to be modified so that calls become asynchronous. This eases the use of the database.

sydneyhauke commented 3 years ago

I should also add that I removed the VolatileDatabase. My reasoning is that now we can persist almost anything in the database. What do you think ?

theo-damiani commented 3 years ago

I should also add that I removed the VolatileDatabase. My reasoning is that now we can persist almost anything in the database. What do you think?

Yes for me we can remove the VolatileDatabase. But this will not break the testing part of some activities if we made calls to Firebase?

EDIT: maybe we need to keep it so we don't have any problem with the others PR like for #150

sydneyhauke commented 3 years ago

I should also add that I removed the VolatileDatabase. My reasoning is that now we can persist almost anything in the database. What do you think?

Yes for me we can remove the VolatileDatabase. But this will not break the testing part of some activities if we made calls to Firebase?

EDIT: maybe we need to keep it so we don't have any problem with the others PR like for #150

We could, but that would imply additional work on this PR. The Database interface has changed a lot. So I would need to both adapt the VolatileDatabase so that it correctly implements the new interface and add additional methods that I also had to add to fully support all operations needed on the database. Otherwise, it's the task of the one responsible for #150 to adapt the Database interface so that it correctly implments what he implemented on the VolatileDatabase. Do you see what I mean ?

YagoGG commented 3 years ago

As for the VolatileDatabase, I am still down for getting rid of it. If other PRs use it they will have to adapt their code (or this PR will have to adapt the codebase, depending on what gets merged first).

sydneyhauke commented 3 years ago

Oh god. I just realized that I might have to adapt every Activity test that might end up communicating with the database... No problem before because we had the VolatileDatabase. But now... I have to mock the database since we are now connected to a real remote database. Sh**.

sydneyhauke commented 3 years ago

Hey @sydneyhauke, thanks a bunch for the changes!

I just made a second batch of functional tests and the todo lists work like a charm.

However, I cannot see the body of the notes I edit. Here are the steps to reproduce:

1. Create a note.

2. Add any text in its body.

3. Hit the green button to save the note.

4. Hit the "back" button to return to the note list.

5. Re-open the note.

6. There is no body.

The expected behaviour would be to see the text you just wrote in step (2).

Oddly enough, the file is updated in Firebase, so I guess that it's an issue either when the file is downloaded, or when we read its contents to populate the view.

Let me know if you need any further details to reproduce, I'll be glad to oblige!

Please also check out the two inline comments I left in my last review, as they have not been addressed yet.

Cheers!

I think I know why the body isn't saved. My current solution saves the body only if the back button (upper left on the screen) is pressed. I should save the body whenever the activity looses focus.

sydneyhauke commented 3 years ago

Actually, @YagoGG, instead of saving the body whenever we click the green button, shouldn't we instead save the body only if the activity looses focus ?

YagoGG commented 3 years ago

That's actually my preference, I was going to suggest to remove the green button in the next sprint. So if we can do it now that's awesome :)

YagoGG commented 3 years ago

If it's easier for you, we can also do it whenever the EditText loses focus. I guess the event handler is also triggered if the activity is finished. Just an idea in case it might make the implementation simpler!

theo-damiani commented 3 years ago

In the database, we have renameTask, setTaskDone, and maybe (in the future) more stuff like this that changes an already existing task. Do you think that it will be easier to have a method updateTask as we have for todo: updateTodoList. And we could have the same for the notes.

sydneyhauke commented 3 years ago

Hey @sydneyhauke, thanks a bunch for the changes!

I just made a second batch of functional tests and the todo lists work like a charm.

However, I cannot see the body of the notes I edit. Here are the steps to reproduce:

1. Create a note.

2. Add any text in its body.

3. Hit the green button to save the note.

4. Hit the "back" button to return to the note list.

5. Re-open the note.

6. There is no body.

The expected behaviour would be to see the text you just wrote in step (2).

Oddly enough, the file is updated in Firebase, so I guess that it's an issue either when the file is downloaded, or when we read its contents to populate the view.

Let me know if you need any further details to reproduce, I'll be glad to oblige!

Please also check out the two inline comments I left in my last review, as they have not been addressed yet.

Cheers!

Last commits should have fixed the issue you described.