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

Show recently created entities in Experimental Settings #5294

Merged
merged 20 commits into from
Nov 9, 2022

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Sep 23, 2022

Closes #5231
Blocked by getodk/javarosa#691
Blocked by getodk/javarosa#694

This goes a bit further than the issue describes and adds an "entitiy browser" UI that can be accessed from Experimental settings. This will show entities created by forms. Entities are not "persisted" and you'll get a clean slate every time you restart the app.

One thing to note is that I decided to use the AndroidX Navigation library to implement the browser UI (you can see the switch in 73a15c8). This was an experiment to see how it felt, and I ended up really liking it! Interested to know what others think.

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

New tests and verified manually.

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

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?

Should just add the ability to view created entities. I think doing a quick check on form entry (filling, saving, editing forms etc) in general would be good, but we don't need to check entity forms themselves as this is still experimental and something we'll be playing with later.

Before submitting this PR, please make sure you have:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@lognaturel lognaturel changed the title Show recently created entities in Expermental Settings Show recently created entities in Experimental Settings Oct 3, 2022
@seadowg seadowg force-pushed the entity-parse branch 2 times, most recently from 78089db to c212917 Compare October 5, 2022 09:04
@seadowg seadowg requested a review from lognaturel October 5, 2022 12:27
@seadowg seadowg marked this pull request as ready for review October 5, 2022 13:02
@seadowg seadowg removed the blocked label Oct 5, 2022
strings/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
<meta>
<instanceID/>
<instanceName/>
<entities:entity entities:dataset="people">
Copy link
Member

Choose a reason for hiding this comment

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

After reading https://www.w3.org/TR/2006/REC-xml-names11-20060816/#defaulting: "Default namespace declarations do not apply directly to attribute names; the interpretation of unprefixed attributes is determined by the element on which they appear." and https://stackoverflow.com/a/46865/137744, we've decided not to namespace the attributes on the entity block. I think I checked for that in the JR pull request and made sure they're not enforced but would be worth a double check and to change this here.

saveto and entities-version get namespaced because they are on nodes that are not "ours".

Copy link
Member Author

@seadowg seadowg Oct 17, 2022

Choose a reason for hiding this comment

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

I think I see what you're getting at, but just to confirm, you mean that dataset shouldn't need a prefix? I'm guessing the inner nodes (create and label) still do however?

This is something we should change in tests in JavaRosa also if so.

strings/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@lognaturel
Copy link
Member

Couple of very small changes to consider. I think this is ready for a mini regression pass around form entry!

@dbemke
Copy link

dbemke commented Oct 11, 2022

I’d like to double-check the expected results -when new entities should appear in "experimental".
I’m able to view created entities in "experimental" when the form is:

  • finalized and sent
  • finalized and not sent (flight mode on)

The entity is not created if the form is saved but not finalized.
Are those results the expected results?

@seadowg
Copy link
Member Author

seadowg commented Oct 11, 2022

Are those results the expected results?

Yup that looks correct to me! Entities are only created once the form is initialized.

@dbemke
Copy link

dbemke commented Oct 11, 2022

Entities are not "persisted" and you'll get a clean slate every time you restart the app.

@seadowg Entities that were saved and visible in experimental settings disappear after killing the app. If I just close the app by tapping device back button, the saved entities are still in experimental settings. By restarting the app you mean e.g. removing/downloading the app?

@seadowg
Copy link
Member Author

seadowg commented Oct 11, 2022

By restarting the app you mean e.g. removing/downloading the app?

No I mean killing the app and starting it up again. I'd usually refer to "removing/downloading the app" as "reinstalling".

@dbemke
Copy link

dbemke commented Oct 11, 2022

Problem description

When a user finalizes and saves an empty one question enity form, the app crashes.

Steps to reproduce the problem

  1. Open one question entity form.
    one-question-entity.xml.txt
  2. Don’t fill anything.
  3. Tap next.
  4. Finalize and save the form.

@seadowg
Copy link
Member Author

seadowg commented Oct 12, 2022

@dbemke good catch. I'll look at fixing that.

@lognaturel
Copy link
Member

From another conversation:

  • Currently empty values should save as blank strings for those entity properties.
  • Once we have entity updates there will be a difference between non-relevant form fields and blank relevant form fields. Non-relevant form fields will be no-ops (so the current entity property value will be used) and blank relevant form fields will result in updates to clear out the corresponding entity property.

The update behavior may not matter now but wanted to include it for completeness!

@lognaturel
Copy link
Member

I have forms that feed into two datasets, people and shovels. I see all of the entities when I navigate to people AND when I navigate to shovels.

@dbemke
Copy link

dbemke commented Oct 13, 2022

shovel_registration.xml.txt

I had a look at the shovel form @lognaturel mentioned and there are different results for the first entity created after downloading a project and following entities.
In the shovel form I filled in:

  • blue 22cm (first submission after downloading a project)
  • purple 33cm
  • black 66cm
    In experimental settings for the first submission there is the color, but for the following submissions length is visible. There is also a difference on Central- the first submission after downloading a project has "meta-entity-label”.
    Is this the way submissions should be shown?
    entities
    shovelscentral

@seadowg
Copy link
Member Author

seadowg commented Oct 13, 2022

When a user finalizes and saves an empty one question enity form, the app crashes.

I'll make changes to fix this in JavaRosa so blocking this PR on that.

I have forms that feed into two datasets, people and shovels. I see all of the entities when I navigate to people AND when I navigate to shovels.

I'm guessing it's two forms rather than one form right? Yeah, it looks I missed actually filtering entities by dataset! I knew this had felt too easy. Definitely a peril of switching back and forth between the two projects...

In experimental settings for the first submission there is the color, but for the following submissions length is visible.

We're only showing the "first" property right now for each entity. It could be that "first" ends up a bit random though in this case. In hindsight, this feels too confusing for multiple property entities (as it really looks like something is going wrong here). I'll have a look at doing the easiest thing for this PR to make it nicer - either making sure always show the same property, or showing all properties. I want to make sure I'm not spending too much time perfecting all this seeing as it's very much a debug tool. We can still do another pass on it before release.

There is also a difference on Central- the first submission after downloading a project has "meta-entity-label”.
Is this the way submissions should be shown?

That's very strange. You should be able to see the value Central is using in the actual submission XML (in Collect's instances dir) in /data/meta/entities:entity/entities:label. Are the values missing there as well? Slack me if you have trouble reading the submissions.

@dbemke
Copy link

dbemke commented Oct 13, 2022

In xml entities:label is filled in only for the first submission.
shovelsxml

@seadowg
Copy link
Member Author

seadowg commented Oct 13, 2022

In xml entities:label is filled in only for the first submission.

Very strange. I'll see if I can reproduce this.

@seadowg
Copy link
Member Author

seadowg commented Oct 13, 2022

Ok it looks like both the different ordering of properties and the missing label are to do with form def caching. @dbemke @srujner an interesting thing I always forget is that Collect will cache the parsed form definition after loading it the first time. This means that we sometimes see bugs that happen only on the first or not on the first time you load the form. I've fixed the properties (and we're now showing all of them to make it easier to see what's going on) and will look into what's happening with the label next.

@seadowg
Copy link
Member Author

seadowg commented Oct 13, 2022

I haven't been able to immediately find a reason that entities:label isn't being populated for forms filled with a cached form def. As far as I understand though, that shouldn't be a special case. The form def just uses existing (non entity specific) features for all that to work and that if that's not working, it's due to an existing bug in JavaRosa. I'm going to try making a non-entity form that demonstrates this.

@seadowg
Copy link
Member Author

seadowg commented Oct 13, 2022

@dbemke @srujner yeah the missing label is an existing problem (getodk/javarosa#695). That shouldn't block this PR. It is something we'll want to fix before release however, as it really messed up entities.

@dbemke
Copy link

dbemke commented Oct 13, 2022

I checked different Android versions and in Android 8.1, 10, 11, 12, 13 the missing part in entities:label for following submissions in xml is present in all Android versions.
In case of entities shown in experimental part:

  • Android 8.1, 10, 11, 12 show color (for the first submission), length (fot the following submissions)
  • Android 13 shows color for all the submissions
    android13entities

@srujner
Copy link

srujner commented Nov 8, 2022

At the moment, everything has been checked by us, but we're waiting for the fix for the issue with scrolling.
Let us now @seadowg when this will be ready. For now, we will remove the "Needs testing" label.

@grzesiek2010
Copy link
Member

@srujner @dbemke I've fixed the problem with scrolling the list and fixed conflicts so please finish testing.

@dbemke
Copy link

dbemke commented Nov 9, 2022

Tested with Success!

Verified on Android 10

Verified Cases:

  • the issues with an empty answer and scrolling down the list of entities are no longer reproducing
  • finalizing a new form and when form is edited
  • two different forms with the same name in a dataset
  • scrolling entities
  • finalizing a finalized form
  • finalizing a form without sending and afterwards deleting the form
  • killing the app
  • killing the app and sending again already sent forms
  • changing the dataset name in a new version of the form
  • hide old form versions disabled + finalizing different version of the same form with different dataset name
  • dark and light mode
  • don’t keep activities enabled/disabled
  • minimizing the app, rotating and locking the screen
  • different Form Management settings

@srujner
Copy link

srujner commented Nov 9, 2022

Tested with Success!

Verified on Android 13

@grzesiek2010 grzesiek2010 merged commit 74940ac into getodk:master Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Finalizing a form that creates an entity should add to the debug log
5 participants