Hide old form versions #2216

grzesiek2010 · 2018-05-17T09:13:35Z

What has been done to verify that this works as intended?

I tested the solution on a real device and I also wrote an automated test.

Why is this the best possible solution? Were any other approaches considered?

Hiding old forms is better than removing them because thanks to this approach we can still edit old saved forms.

Are there any risks to merging this code? If so, what are they?

We only hide old forms keeping them in the database. That means old saved forms are still available what makes this change less risky. I wrote an automated test so I think it's not risky and we hide old forms in an appropriate way.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

getodk/docs#710

Before submitting this PR, please make sure you have:

run ./gradlew pmd checkstyle lint findbugs and confirmed all checks still pass.
verified that any new UI elements use theme colors. UI Components Style guidelines

lognaturel · 2018-05-21T23:02:01Z

collect_app/src/main/java/org/odk/collect/android/dao/FormsDao.java

@@ -171,52 +171,48 @@ public int updateForm(ContentValues values, String where, String[] whereArgs) {

    public List<Form> getFormsFromCursor(Cursor cursor) {
        List<Form> forms = new ArrayList<>();
-        if (cursor != null) {


Can you please describe why you've made this change? They look mostly equivalent except that in the new version the cursor is not closed.

It's because the cursor is somehow reused (no created again) if we go back from a survey (for example after saving) so I can't close that cursor because then I'm not able to read rows again.

Btw I need to investigate why we are moved back to forms list after saving a form it was always the main menu as I remember...

Got it, thanks! That kind of info is perfect for a commit message description.

😭 We did actually start having a conversation about this change, we just didn't go deep enough. (See #2687 (comment) for later implications)

lognaturel · 2018-05-21T23:04:57Z

collect_app/src/main/java/org/odk/collect/android/utilities/FormUtils.java

+                        FormsProviderAPI.FormsColumns.DATE}
+        );
+
+        for (Form form : forms) {


This is n² in the number of forms. In practice it probably doesn't make a visible difference (maybe on slower devices with lots of forms?) but best to avoid if possible.

lognaturel · 2018-05-21T23:22:18Z

This currently uses numerically sorted version and then date/time entered in forms database in case of null versions to identify the latest form. I propose we sort just by date/time entered in the forms database instead. I can't think of a case in which a user wouldn't want to show just the version they last moved to the device.

The current approach has a few problems:

It doesn't work with servers like Ona that don't guarantee sorted versions. By default they use a GUID for the version.
It doesn't work with Aggregate which sorts versions lexicographically rather than numerically. For example, form 10 would come before form 2 (think of alphabetical sort in which the left-most character is first used).

Additionally, sorting using both columns makes the code more complex. If you were just using date/time you could do it through the query, right, @grzesiek2010?

Apologies for only thinking of these things now!

@yanokwa and others, does only showing the latest version transferred to the device make sense?

lognaturel · 2018-05-21T23:29:25Z

One "bad" behavior if only database created_at is used: if the user sets their clock back in time, any form update they download or transfer to their device will be hidden. For example: my device year is set to 2021. I download some forms. I notice my year is wrong and set it to 2018. I won't see any updates to the existing forms. This is a rare enough situation that I'm not worried about it.

No matter what, I do think we need to provide a way for users to show all versions. That would be a good setting to add to the Form update group in Form management settings.

yanokwa · 2018-05-21T23:34:55Z

I think the proposed change to the current PR is necessary.

We don't want to break compatibility with Ona and the risk for a user clock going backward in time and not being able to see the forms in the future is fixable: delete the blank form.

lognaturel

only use created_at date/time to determine latest form
add setting to show all form versions

I don't think the version comparison is working anyway. If I add version 3, I see version 3. If I then add version 13, I see only version 13. If I then add version 4, I see only version 4. That is the desired behavior -- only the latest form to be added is shown.

lognaturel · 2018-05-22T01:11:30Z

@grzesiek2010 If you are able to address those issues during your day Tuesday, please tag as needs testing and high priority so we can try to get it in and keep the ❄️freeze ❄️on track! 🙏

grzesiek2010 · 2018-05-22T10:00:56Z

@opendatakit-bot label "needs testing"
@opendatakit-bot label "high priority"

@lognaturel it's ready I'm not sure about the setting whether it should be selected by default or nor but I marked the pr as needs testing as you asked. We can fix that in the evening if needed.

mmarciniak90 · 2018-05-22T13:53:47Z

Tested with success!

Verified on Android: 4.1, 4.2, 4.4, 5.1, 6.0, 7.0, 8.1

Verified cases:

hide old versions
show old versions
edit form with old version
send a form with old version
import settings
clean setting

mmarciniak90 · 2018-05-22T13:54:27Z

@opendatakit-bot label "behavior verified"
@opendatakit-bot unlabel "needs testing"

lognaturel · 2018-05-22T17:55:08Z

@grzesiek2010 I just realized that when I transferred different versions directly with adb only one version was on the device so your original implementation was in fact as you initially intended.

But using just created_at will work with all the different version conventions used across the ecosystem so thanks for making that quick fix!

lognaturel reviewed May 21, 2018

View reviewed changes

lognaturel requested changes May 21, 2018

View reviewed changes

grzesiek2010 added 5 commits May 22, 2018 10:29

Add new setting to hide old form versions

63ce18f

Updated Form class

ddee691

Hide old form versions

f958fe8

Added an automated test

a7770ce

Fixed a comment

b2fa94e

grzesiek2010 force-pushed the COLLECT-1694 branch from dd5a3fe to b2fa94e Compare May 22, 2018 09:38

Respect settings

2f33bf8

getodk-bot added needs testing high priority Should be looked at before other PRs/issues labels May 22, 2018

getodk-bot added behavior verified and removed needs testing labels May 22, 2018

Update strings.xml

7cf4514

lognaturel approved these changes May 22, 2018

View reviewed changes

lognaturel merged commit f196877 into getodk:master May 22, 2018

This was referenced Oct 25, 2018

Don't close a cursor in FormsDao#getFormsFromCursor() since we may need to reuse it #2687

Closed

Use query to get latest versions of forms #2691

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide old form versions #2216

Hide old form versions #2216

grzesiek2010 commented May 17, 2018 •

edited

Loading

lognaturel May 21, 2018

grzesiek2010 May 22, 2018

lognaturel May 22, 2018

lognaturel Oct 25, 2018

lognaturel May 21, 2018

lognaturel commented May 21, 2018 •

edited

Loading

lognaturel commented May 21, 2018 •

edited

Loading

yanokwa commented May 21, 2018

lognaturel left a comment •

edited

Loading

lognaturel commented May 22, 2018

grzesiek2010 commented May 22, 2018

mmarciniak90 commented May 22, 2018 •

edited

Loading

mmarciniak90 commented May 22, 2018

lognaturel commented May 22, 2018

Hide old form versions #2216

Hide old form versions #2216

Conversation

grzesiek2010 commented May 17, 2018 • edited Loading

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

Are there any risks to merging this code? If so, what are they?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

lognaturel May 21, 2018

Choose a reason for hiding this comment

grzesiek2010 May 22, 2018

Choose a reason for hiding this comment

lognaturel May 22, 2018

Choose a reason for hiding this comment

lognaturel Oct 25, 2018

Choose a reason for hiding this comment

lognaturel May 21, 2018

Choose a reason for hiding this comment

lognaturel commented May 21, 2018 • edited Loading

lognaturel commented May 21, 2018 • edited Loading

yanokwa commented May 21, 2018

lognaturel left a comment • edited Loading

Choose a reason for hiding this comment

lognaturel commented May 22, 2018

grzesiek2010 commented May 22, 2018

mmarciniak90 commented May 22, 2018 • edited Loading

mmarciniak90 commented May 22, 2018

lognaturel commented May 22, 2018

grzesiek2010 commented May 17, 2018 •

edited

Loading

lognaturel commented May 21, 2018 •

edited

Loading

lognaturel commented May 21, 2018 •

edited

Loading

lognaturel left a comment •

edited

Loading

mmarciniak90 commented May 22, 2018 •

edited

Loading