Add identify-user support #3439

seadowg · 2019-11-06T17:45:09Z

Closes #3435. This implements the feature described (with designs and specs) in that issue.

A couple of notes on implementation:

I've reworked a little about the structure of audit code so it follows producer-consumer design. I've kept the AuditEventLogger (producer) but it now defines an AuditEventWriter interface that's implemented with an AsyncTask. The goal here was to seperate out the logic that creates the list of events from the writing of those events to a file.
I've moved code that just deals with auditing into the audit package. This is talked about in the State of the Union.
Showing the dialog had to happen in the FormEntryActivity as it controls loading and rendering the form. The code that makes that happen isn't great but I want to follow up with a bit of a refactor of the Activity itself. I didn't feel including that would be the right move as it would be a lot of noise for a reviewer.

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

Local testing and submitting the form to Aggregate.

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?

There shouldn't be any changes other than the new feature. Would be good to test form entry in general to make sure we haven't messed up any existing flows.

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

grzesiek2010 · 2019-11-14T15:00:10Z

There is no ok/navigate forward button?

It's strange that I have to open the keyboard and click Done.

The keyboard moves the toolbar up and it's mixed with the status bar:
This white-gray area won't be well visible when a user is outside in a sunny day (especially the Identity text and the cursor)
I think it's strange that once you open a form the cursor is blinking but the keyboard is hidden. Either the cursor and keyboard should be visible or both should be hidden.
If you rotate your device when that new view is visible the form is closed then you can open it again bypassing the view 😄
We should handle commas, if I use a comma entering my identity the answer will end up in two separate columns:
I'm able to enter just one space char and go further. We shouldn't allow doing this I guess. The same with emoticons.
If you minimize the app (seeing the new view) and open it again the view is skipped (similar case to 5 but not the same).
Dark theme requires improvements:

seadowg · 2019-11-14T15:23:09Z

@grzesiek2010 all great finds! Can you stick it into "Request changes"? I'll have a look at fixes.

collect_app/src/main/res/menu/menu_ak.xml

seadowg · 2019-11-14T16:56:03Z

Just questions on a couple of your points @grzesiek2010 (most of them clearly just need fixed):

There is no ok/navigate forward button?

Intention was just to have the keyboard "done" take care of this but that's fairly useless without the field focusing (as you point out) - I'll fix that. Do you think we still need a button if we have that? I was following the pattern used by password prompts on Android.

I'm able to enter just one space char and go further. We shouldn't allow doing this I guess. The same with emoticons.

Great point. The spec states a "non-blank identifier" is required. I don't want to add too much validation in but I'd assume not allowing whitespace only would be ok. Do you have any extra contect on this @lognaturel? If not I can dig through the forum discussion.

seadowg · 2019-11-14T17:12:00Z

I've updated the acceptance in #3435 based on @grzesiek2010's findings

grzesiek2010 · 2019-11-14T18:53:55Z

Intention was just to have the keyboard "done" take care of this but that's fairly useless without the field focusing (as you point out) - I'll fix that. Do you think we still need a button if we have that?

I would prefer to have such a button but it's my personal opinion.

seadowg · 2019-11-19T17:18:49Z

Ok I think I've fixed everything @grzesiek2010 found. Some annoying stuff around Theming and getting the soft keyboard to show held this up a little. I'm unclear on one path into the form that I'll leave a comment on.

collect_app/src/main/java/org/odk/collect/android/activities/FormEntryActivity.java

Generally mocking static methods is considered a smell as it's sign of implicit dependencies in the object under a test. Here it was particularly confusing as Powermock was being used to implicitly set the system to 0 so that tests would pass.

This makes it easier to work on audit features as tests can be run as a set and it's simple to see all the classes involved in audit logging. Also moved AuditEventSaveTastTest into Robolectric from Espresso.

- Introduced an AuditEventWriter abstraction that wraps AuditEventSaveTask - Test AuditEventLogger by inspecting the AuditEvents it writes to the AuditEventWriter - Remove dependency on Collect static method

…ty creation

lognaturel

The code looks good to me and the functionality looks like it's all there thanks to @grzesiek2010's initial review! 🙏

I feel like I'm being a broken record but the fonts really look really small to me. I would expect this to lead to occasional inaccurate identity entry because it's just hard to see what's being typed in. BUT. I understand that they are default and match where the world is going so I think we should go with it and see what user feedback comes in.

Like @grzesiek2010, I instinctively want to swipe forward and had to think a second about tapping "Done". I would tend to have some kind of explicit "Save" button but I'm ok with waiting for user feedback on that part as well.

lognaturel

The code looks good to me and the functionality looks like it's all there thanks to @grzesiek2010's initial review! 🙏

I feel like I'm being a broken record but the fonts really look really small to me. I would expect this to lead to occasional inaccurate identity entry because it's just hard to see what's being typed in. BUT. I understand that they are default and match where the world is going so I think we should go with it and see what user feedback comes in.

Like @grzesiek2010, I instinctively want to swipe forward and had to think a second about tapping "Done". I would tend to have some kind of explicit "Save" button but I'm ok with waiting for user feedback on that part as well.

lognaturel

The code looks good to me and the functionality looks like it's all there thanks to @grzesiek2010's initial review! 🙏

I feel like I'm being a broken record but the fonts really look really small to me. I would expect this to lead to occasional inaccurate identity entry because it's just hard to see what's being typed in. BUT. I understand that they are default and match where the world is going so I think we should go with it and see what user feedback comes in.

Like @grzesiek2010, I instinctively want to swipe forward and had to think a second about tapping "Done". I would tend to have some kind of explicit "Save" button but I'm ok with waiting for user feedback on that part as well.

mmarciniak90 · 2019-11-25T13:57:32Z

@seadowg Here is what I found

The one whitespace is still accepted to go further - the problem reported by Grzegorz is visible
The status bar is not visible on Android 4.4 and 4.2

Anroid 6.0	Android 4.4

Copy-paste functionality covers the field. Some extra space is visible near the cursor pointer and 'frame' for Cup/Copy/Clipboard/Share options. Hope, you can see in on screens:

I also have questions:

Should the font size changes be applied to the identity page?
I changed the language to Spanish and I still this page in English. I understand that translations are not updated but I want to make sure that it is not missed.

mmarciniak90 · 2019-11-25T14:02:48Z

@opendatakit-bot unlabel "needs testing"

seadowg · 2019-11-25T16:04:43Z

@mmarciniak90 thanks for these - I'll have a look at fixing these issues. I talked to a form designer today about the feature and they weren't conncerned about whitespace but given it says "non blank" in the spec I think we should block whitespace only entries. On the copy/pate menu covering the field: I'm not sure we have control over that but I'll have a look.

On your questions:

Should the font size changes be applied to the identity page?

I'm guessing you mean font size changes in the app rather than at the system level? I'm actually not sure where the distinction lies for what should react to the app's font size change. @lognaturel?

I changed the language to Spanish and I still this page in English. I understand that translations are not updated but I want to make sure that it is not missed.

Good point. What is our normal process for getting translations in place?

lognaturel · 2019-11-25T16:32:43Z

where the distinction lies for what should react to the app's font size change

It only affects the form entry activity currently. It feels very weird because it doesn't include the hierarchy/jump view or the save/submit screen. Ideally I would think both would be affected because they feel like they're part of the form filling experience. I think that ideally font changes would go to this screen but I don't currently feel strongly about it given the chaotic current state. It's already weird that its fonts don't match the rest of the app (they match Material Design standards which is certainly better than matching nothing).

What is our normal process for getting translations in place?

https://github.com/opendatakit/collect#contributing-translations outlines the process. Transifex pulls the strings.xml file from master once a day so as soon as this is merged, the strings that need translating will be pulled in. The translations will be updated once on ~Dec 4 for a beta and again before release.

Translations are a great thing to check, @mmarciniak90. I believe that it is now impossible to commit untranslatable strings because the build will break. But maybe what you can also do is quickly verify the strings.xml file to make sure all strings that you expect are there. I also do this during code review.

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

We don't need its behaviour for this screen and it just causes problems with the Toolbar being hidden on lower API levels.

seadowg · 2019-11-25T21:24:55Z

@mmarciniak90 1 and 2 are fixed.

3 is proving amazingly difficult. I'm going to need to some investigation outside our app (in a demo codebase) to work out what's going on. We're going to use the same full screen dialog pattern for the next feature I work (Track Changes Reason) on so I'd vote for us to move on and try and fix after that is done. If you agree I'll make an issue once we merge this is.

EDIT: Scratch that. Had a sudden brainwave and managed to fix 3. Still not 100% sure I understand how exactly but it's looking good!

For some reason this was setting the pop up background that appears on long press on an EditText.

mmarciniak90 · 2019-11-26T09:42:35Z

@seadowg thanks for taking care of the mentioned problems. I can confirm that they are fixed.

I noticed one more thing.

User adds identity
User rotates device
Filled identity disappears

It would be better not to lose this data during rotation, what do you think?

seadowg · 2019-11-26T10:11:00Z

It would be better not to lose this data during rotation, what do you think?

That definitely shoudn't be happening. I'll take a look.

seadowg · 2019-11-26T11:38:55Z

@mmarciniak90 fixed! Thanks for keeping me on my toes around screen rotations (this goes for @grzesiek2010 too). It's probably very obvious I've managed to work mostly on apps that lock to portrait in the past 😉

mmarciniak90 · 2019-11-26T13:40:58Z

Tested with success

Tested on Android: 4.2, 4.4, 5.1, 6.0, 6.0, 8.1, 9.0, 10.0

Verified cases:

Blank identity
Identity with whitespace only
Identity with chars and whitespace
Status bar
Copy-paste functionality
Username in audit.csv file
Light/dark mode
Screen rotation

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

seadowg force-pushed the identify-user branch 2 times, most recently from 2de0585 to 8d58db0 Compare November 12, 2019 14:17

seadowg marked this pull request as ready for review November 13, 2019 14:25

grzesiek2010 reviewed Nov 14, 2019

View reviewed changes

collect_app/src/main/res/menu/menu_ak.xml Outdated Show resolved Hide resolved

seadowg force-pushed the identify-user branch from 8d58db0 to 67db47e Compare November 14, 2019 18:02

seadowg force-pushed the identify-user branch from 23703f1 to e56a95c Compare November 19, 2019 14:22

seadowg commented Nov 19, 2019

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/activities/FormEntryActivity.java Outdated Show resolved Hide resolved

seadowg added 17 commits November 20, 2019 18:28

Remove Powermock from AuditEventLoggerTest

3a43e21

Generally mocking static methods is considered a smell as it's sign of implicit dependencies in the object under a test. Here it was particularly confusing as Powermock was being used to implicitly set the system to 0 so that tests would pass.

Move audit code into one package

40caeb7

This makes it easier to work on audit features as tests can be run as a set and it's simple to see all the classes involved in audit logging. Also moved AuditEventSaveTastTest into Robolectric from Espresso.

Add note to state of union about package restructuring

89e0318

Remove test only helpers on AuditEventLogger

4b664e2

- Introduced an AuditEventWriter abstraction that wraps AuditEventSaveTask - Test AuditEventLogger by inspecting the AuditEvents it writes to the AuditEventWriter - Remove dependency on Collect static method

User is prompted to identify themselves when form has identify-user

42d48b5

Don't prompt the user to identify themselves when identify-user=false

01a8c24

User identity is required to not be empty

41b6773

Form is closed if user dismisses identity prompt

b1f6cb1

Set user on AuditEvent during logging

086d1c4

Log user to audit.csv

f719c4d

Add close button to identity prompt dialog

6bf2cf6

Try to make FormEntryActivity code a little nicer

09371a3

Refactor to allow IdentityPromptViewModel to be initialized on Activi…

42041d8

…ty creation

Test AsyncEventAuditEventWriter instead of task

ff634ef

Update upgrade path test for audit event writing

edcefa8

Account for null auditConfig in FormController

b4da96f

Stop identity dialog closing on rotate and background

0bfcad8

seadowg changed the title ~~[WIP] Add identify-user support~~ Add identify-user support Nov 21, 2019

seadowg requested a review from lognaturel November 21, 2019 10:41

lognaturel approved these changes Nov 22, 2019

View reviewed changes

seadowg added the needs testing label Nov 22, 2019

grzesiek2010 approved these changes Nov 22, 2019

View reviewed changes

getodk-bot removed the needs testing label Nov 25, 2019

Don't allow whitespace users

0ee4868

lognaturel reviewed Nov 25, 2019

View reviewed changes

collect_app/src/main/java/org/odk/collect/android/utilities/StringUtils.java Show resolved Hide resolved

seadowg added 3 commits November 25, 2019 18:00

Make sure toolbar is visible on older API levels

6bba809

Rename TextUtils to StringUtils

9ca2a5c

Switch out CoordinatorLayout from full screen dialog

c8deaa7

We don't need its behaviour for this screen and it just causes problems with the Toolbar being hidden on lower API levels.

seadowg added the needs testing label Nov 25, 2019

Remove background from dialog theme

ed4a633

For some reason this was setting the pop up background that appears on long press on an EditText.

Make sure identity is persisted during rotations

7b69272

seadowg force-pushed the identify-user branch from 59d8f10 to 7b69272 Compare November 26, 2019 11:37

getodk-bot added behavior verified and removed needs testing labels Nov 26, 2019

grzesiek2010 merged commit 0a6bb3d into getodk:master Nov 26, 2019

seadowg mentioned this pull request Dec 9, 2019

Add documentation for Collect 1.25 audit features getodk/docs#1155

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add identify-user support #3439

Add identify-user support #3439

seadowg commented Nov 6, 2019 •

edited

Loading

grzesiek2010 commented Nov 14, 2019 •

edited

Loading

seadowg commented Nov 14, 2019

seadowg commented Nov 14, 2019

seadowg commented Nov 14, 2019

grzesiek2010 commented Nov 14, 2019

seadowg commented Nov 19, 2019

lognaturel left a comment

lognaturel left a comment

lognaturel left a comment

mmarciniak90 commented Nov 25, 2019

mmarciniak90 commented Nov 25, 2019

seadowg commented Nov 25, 2019 •

edited

Loading

lognaturel commented Nov 25, 2019 •

edited

Loading

seadowg commented Nov 25, 2019 •

edited

Loading

mmarciniak90 commented Nov 26, 2019

seadowg commented Nov 26, 2019

seadowg commented Nov 26, 2019

mmarciniak90 commented Nov 26, 2019

Add identify-user support #3439

Add identify-user support #3439

Conversation

seadowg commented Nov 6, 2019 • edited Loading

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

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?

Before submitting this PR, please make sure you have:

grzesiek2010 commented Nov 14, 2019 • edited Loading

seadowg commented Nov 14, 2019

seadowg commented Nov 14, 2019

seadowg commented Nov 14, 2019

grzesiek2010 commented Nov 14, 2019

seadowg commented Nov 19, 2019

lognaturel left a comment

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

lognaturel left a comment

Choose a reason for hiding this comment

mmarciniak90 commented Nov 25, 2019

mmarciniak90 commented Nov 25, 2019

seadowg commented Nov 25, 2019 • edited Loading

lognaturel commented Nov 25, 2019 • edited Loading

seadowg commented Nov 25, 2019 • edited Loading

mmarciniak90 commented Nov 26, 2019

seadowg commented Nov 26, 2019

seadowg commented Nov 26, 2019

mmarciniak90 commented Nov 26, 2019

seadowg commented Nov 6, 2019 •

edited

Loading

grzesiek2010 commented Nov 14, 2019 •

edited

Loading

seadowg commented Nov 25, 2019 •

edited

Loading

lognaturel commented Nov 25, 2019 •

edited

Loading

seadowg commented Nov 25, 2019 •

edited

Loading