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

Add hidden-answer appearance for barcode questions #5870

Merged
merged 5 commits into from
Dec 15, 2023

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Dec 13, 2023

As discussed at https://forum.getodk.org/t/addition-of-minimal-or-no-answer-appearance-for-question-types-that-launch-an-external-selector/41151/3.

This adds support for a new hidden-answer appearance for barcode questions. If present, barcode contents will never be shown under the "Scan Barcode"/"Replace Barcode" button. It might be that we want to add some indication that the barcode has been scanned (other than the button changing to "Replace Barcode"). For the moment, the intention is for it to be used with a note on the same screen to display a formatted version of the barcode contents.

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

New tests and verified manually.

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 new appearance. It'd be good to check that the barcode still behaves correctly without the appearance. Other than, not much has been touched.

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

Any form with barcode (and the new appearance).

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

getodk/docs#1723

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • 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

@seadowg seadowg changed the title Add no-answer appearance for barcode questions Add no-answer appearance for barcode questions Dec 13, 2023
@seadowg seadowg marked this pull request as ready for review December 13, 2023 12:44
@@ -69,6 +69,7 @@ object Appearances {
const val NEW_FRONT = "new-front"
const val NEW = "new"
const val FRONT = "front"
const val NO_ANSWER = "no-answer"
Copy link
Member

Choose a reason for hiding this comment

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

no answer sounds confusing to me. The goal is to still keep it but not display so I think something like hidden-answer, protected-answer, secret-answer could be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah @lognaturel weren't sure how we felt about it when talked yesterday. hidden-answer is so much better thanks!

widgetValueChanged();
}

private void updateVisibility() {
if (hasAppearance(getFormEntryPrompt(), Appearances.NO_ANSWER)) {
binding.barcodeAnswerText.setVisibility(GONE);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to display ******* for example so that it's clear the answer is still there but protected.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is intended for use with another question on the same screen (in a field-list) displaying the data so I don't think we'd need that. The goal here was to reduce the noise, not protect the scanned data. I think the responsibility to make it clear that the answer is scanned would be moved to the form designer here (as they're opting in to hide the answer). Good to get @lognaturel's thoughts as well!

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok I thought that the appearance could be used for having protected/secret questions too but there should be a separate one for cases like that and then displaying ******* would make sense.
In the case described on the forum, it's better to display nothing, We should make sure this is clear in the documentation that the new appearance is for hiding questions but not making them protected.

widgetValueChanged();
}

private void updateVisibility() {
if (hasAppearance(getFormEntryPrompt(), Appearances.NO_ANSWER)) {
Copy link
Member

Choose a reason for hiding this comment

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

If we do not display the answer in the widget we should not display it in the hierarchy view as well I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I feel like it's ok to leave the data in the hierarchy right now as we can improve this later if people need it to hide there as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah especially if it's just for hiding questions not making them protected it's not that important.

@seadowg seadowg changed the title Add no-answer appearance for barcode questions Add hidden-answer appearance for barcode questions Dec 14, 2023
@lognaturel
Copy link
Member

Thanks for the naming improvement, @grzesiek2010! The behavior matches my understanding of the requirements. Let's get it in beta!

@lognaturel
Copy link
Member

@seadowg can you please update the forum thread with the new proposed appearance name to reduce confusion?

@seadowg seadowg merged commit 62567e1 into getodk:master Dec 15, 2023
6 checks passed
@seadowg seadowg removed the request for review from lognaturel December 15, 2023 10:54
@seadowg seadowg deleted the no-answer branch December 15, 2023 10:55
@dbemke
Copy link

dbemke commented Jan 8, 2024

@seadowg We have a few questions to the PR (what should be tested) and how the new appearance should work:

  • should we test only the barcode widget with the appearance hidden-answer or should we also test other widgets behavior with the appearance (e.g. date picker as mentioned at the forum)
  • the hidden answer isn’t shown in the question but it’s shown in the hierarchy view (according to conversation in the PR)
  • at the forum you mentioned "This could then be used in a field-list with a note question (using a calculation) to display a formatted version of the data” – is there a form like that which we can use for testing or it doesn’t need to be tested

@seadowg
Copy link
Member Author

seadowg commented Jan 8, 2024

should we test only the barcode widget with the appearance hidden-answer or should we also test other widgets behavior with the appearance (e.g. date picker as mentioned at the forum)

It should only work for barcode questions for the moment, but no harm in testing that everything still works as expected for other widget types.

the hidden answer isn’t shown in the question but it’s shown in the hierarchy view (according to conversation in the PR)

Good catch. We discussed that earlier here and decided that's ok.

at the forum you mentioned "This could then be used in a field-list with a note question (using a calculation) to display a formatted version of the data” – is there a form like that which we can use for testing or it doesn’t need to be tested

I don't think that specific use case needs to be tested, but it'd definitely be nice to try it out in a field-list.

@dbemke
Copy link

dbemke commented Jan 12, 2024

Tested with Success!

Verified on a device with Android 10

Verified cases:

  • barcode widget with hidden appearance, barcode widget with hidden appearance required
  • regression checks in barcodes and QR codes (scanning and reconfiguring)
  • trying to apply the hidden appearance in the date widget

@srujner
Copy link

srujner commented Jan 12, 2024

Tested with Success!

Verified on a device with Android 13

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.

None yet

5 participants