Remove empty instanceDir if a user quits the identity dialog #6020
Conversation
grzesiek2010
force-pushed
the
COLLECT-5921
branch
from
b08c5a1
to
8d7a20d
Compare
| @@ -72,4 +75,8 @@ private static boolean userIsValid(String user) { | |||
| public String getFormTitle() { | |||
| return formName; | |||
| } | |||
|
|
|||
| public void exit() { | |||
| FileUtils.purgeMediaPath(instanceFolder); | |||
There was a problem hiding this comment.
I'm wondering if there's more of a shared place we can handle this clean up. It looks like you can't call FormSaveViewModel#ignoreChanges in this scenario as the FormSaveViewModel won't know about the FormController yet.
Where does this instance folder actually get created?
There was a problem hiding this comment.
Where does this instance folder actually get created?
I'm wondering if there's more of a shared place we can handle this clean up.
I was also thinking about that and I'm aware of the fact that this solution is maybe not perfect but I wasn't able to find a better one. If there is a form that requires identity we don't even save formController before passing it formControllerAvailable so it's not available in other places to read the dir parth and remove.
There was a problem hiding this comment.
Maybe we could pull a chunk of the code in FormSaveViewModel#ignoreChanges out into a use case that can be called in both places? Thinking about that
I'm actually wondering whether if the clean up code in ignoreChanges should just be called whenever we end form filling? It might be that we could start thinking of FormSessionRepository more like one of our data services and move logic like that in there (it could be triggered from clear for example).
Maybe I'm thinking about this incorrectly, but the bug doesn't feel high priority enough to not take these opportunities to restructure while fixing it.
There was a problem hiding this comment.
FormSaveViewModel as the name suggest is responsible for saving-related operations. using it in this case where we don't even enter the form-filling view sounds a little bit weird to me. Do you really think it would be a better place?
There was a problem hiding this comment.
I was more meaning the shared code would move out FormSaveViewModel to a separate use case rather than using it in both places.
| @@ -72,4 +75,8 @@ private static boolean userIsValid(String user) { | |||
| public String getFormTitle() { | |||
| return formName; | |||
| } | |||
|
|
|||
| public void exit() { | |||
| FileUtils.purgeMediaPath(instanceFolder); | |||
There was a problem hiding this comment.
Maybe we could pull a chunk of the code in FormSaveViewModel#ignoreChanges out into a use case that can be called in both places? Thinking about that
I'm actually wondering whether if the clean up code in ignoreChanges should just be called whenever we end form filling? It might be that we could start thinking of FormSessionRepository more like one of our data services and move logic like that in there (it could be triggered from clear for example).
Maybe I'm thinking about this incorrectly, but the bug doesn't feel high priority enough to not take these opportunities to restructure while fixing it.
grzesiek2010
force-pushed
the
COLLECT-5921
branch
3 times, most recently
from
1a13fab
to
ded9bd9
Compare
grzesiek2010
force-pushed
the
COLLECT-5921
branch
from
ded9bd9
to
7f3f940
Compare
Closes #5921
Why is this the best possible solution? Were any other approaches considered?
I think adding the
exitmethod toIdentityPromptViewModelis a good solution because it is consistent with what we does for another ViewModel (FormEntryViewModel).How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
This should only fix the issue and have no side effects.
Do we need any specific form for testing your changes? If so, please attach one.
Any form with Enumerator identification
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still pass