Validation workflow on card

[TomerPacific]

Fixes #1149

Added logic and string literal, called card_name_is_empty, to allow to show the hint in the edit text when creating/editing a card.

Translated the value to almost all languages.

[TheLastProject]

Okay, I finally have some energy to look at stuff again. Thanks for your patience :)

Looking at this, it is a good start but there are few small things:

1. Could you please use getError() to make it so the check isn't duplicated like in

   Android/app/src/main/java/protect/card_locker/ManageGroupsActivity.java

   Lines 161 to 166 in 1779aef

   	CharSequence error = input.getError();
   	if (error != null) {
   	Toast.makeText(getApplicationContext(), error.toString(), Toast.LENGTH_SHORT).show();
   	return;
   	}

   ?
2. Please only add translations you actually speak, I want to avoid Google Translate level translations.
3. I guess noStoreError can go with this new translation
4. cardId and validBalance also get checked in doSave, it would be nice if both of those also used this setError method to keep the UI consistent

[TomerPacific]

@TheLastProject , hope you had a nice vacation.
Wanting to verify the points you listed:

1. Do you want me to use getError on storeFieldEdit inside the onTextChanged?
2. I'll revert all the languages I don't speak (kind of wish I didn't invest all the time to translate it for this)
3. I don't understand what the relevance of noStoreError has to do with group name verification?

[TheLastProject]

1. Do you want me to use getError on storeFieldEdit inside the onTextChanged?

No, inside the onSave as that is what checks if everything is valid.

2. I'll revert all the languages I don't speak (kind of wish I didn't invest all the time to translate it for this)

Thanks 😅

3. I don't understand what the relevance of noStoreError has to do with group name verification?

It doesn't, but it seems ugly to me if in the edit view of the 3 elements that can have an invalid value only 1 ever uses the red exclamation point design. It can also confuse users if different fields on the same screen reacts in a different way to having an invalid value. I guess we can skip that but I don't want to release an update if the behaviour is inconsistent so I'll have to do the other 2 fields myself then before release :)

[TomerPacific]

@TheLastProject , thanks for the clarifications.

I usually think it is best to keep the PR relevant to the issue and it isn't quite apparent that you want other things to be done in the same PR.

I'll work on those changes and post a comment here when I'm done.

[TomerPacific]

@TheLastProject , I have made the changes you requested.
Regarding the build failing, it is doing that on the unit test phase.
When I ran the specific unit tests that failed locally, they passed without an issue.

[TomerPacific]

@TheLastProject , I have made some adjustments to the code and removed the logic you highlighted, but am still seeing the same tests failing.

1. protect.card_locker.LoyaltyCardViewActivityTest > startWithoutParametersCaptureBarcodeCreateLoyaltyCard FAILED java.lang.AssertionError at LoyaltyCardViewActivityTest.java:163
2. protect.card_locker.LoyaltyCardViewActivityTest > removeBarcodeFromLoyaltyCard FAILED java.lang.AssertionError at LoyaltyCardViewActivityTest.java:163
3. protect.card_locker.LoyaltyCardViewActivityTest > startLoyaltyCardWithoutColorsSave FAILED java.lang.AssertionError at LoyaltyCardViewActivityTest.java:163
4. protect.card_locker.LoyaltyCardViewActivityTest > startLoyaltyCardWithExplicitNoBarcodeSave FAILED java.lang.AssertionError at LoyaltyCardViewActivityTest.java:163

As you can see all of them are probably failing for the same exact reason. I'm trying to investigate why.

[TheLastProject]

Hmm, I'm not quite sure why the tests are failing. But given the change still has bugs I am not very upset the tests are currently failing. The tests may become an issue later though...

[TomerPacific]

@TheLastProject ?

[TheLastProject]

I'm not sure what you're expecting from me. The tests are failing, so this can't be merged.

[TomerPacific]

@TheLastProject,
the tests are failing because of the fact that the logic for an empty cardId has been changed in this PR.

If you take a look at startWithoutParametersCaptureBarcodeCreateLoyaltyCard test for example, you can see that:

 @Test
    public void startWithoutParametersCaptureBarcodeCreateLoyaltyCard() throws IOException, ParseException {
        registerMediaStoreIntentHandler();

        ActivityController activityController = Robolectric.buildActivity(LoyaltyCardEditActivity.class).create();
        activityController.start();
        activityController.visible();
        activityController.resume();

        Activity activity = (Activity) activityController.get();
        final Context context = activity.getApplicationContext();

        checkAllFields(activity,
                ViewMode.ADD_CARD,
                "",
                "",
                context.getString(R.string.anyDate),
                context.getString(R.string.never),
                "0",
                context.getString(R.string.points),
                "", //// <------------- Empty card Id
                context.getString(R.string.sameAsCardId),
                context.getString(R.string.noBarcode),
                null,
                null);

        // Complete barcode capture successfully
        captureBarcodeWithResult(activity, true);
        activityController.resume();

        checkAllFields(activity,
                ViewMode.ADD_CARD,
                "",
                "",
                context.getString(R.string.anyDate),
                context.getString(R.string.never),
                "0",
                context.getString(R.string.points),
                BARCODE_DATA,   //// <----------- Not empty cardId
                context.getString(R.string.sameAsCardId),
                BARCODE_TYPE.prettyName(),
                null,
                null);

        shadowOf(getMainLooper()).idle();

        // Save and check the loyalty card
        saveLoyaltyCardWithArguments(activity, "store", "note", context.getString(R.string.anyDate), context.getString(R.string.never), new BigDecimal("0"), context.getString(R.string.points), BARCODE_DATA, context.getString(R.string.sameAsCardId), BARCODE_TYPE.name(), true);

And the test fails from this assertion.

This is again, because the logic has been changed if the cardId is empty.

 cardIdFieldView.setOnFocusChangeListener(new View.OnFocusChangeListener() {
            @Override
            public void onFocusChange(View view, boolean hasFocus) {
                if (!hasFocus && cardIdFieldView.getText().length() == 0) {
                    cardIdFieldView.setError(getString(R.string.noCardIdError));
                }
            }
        });

And,

 if (cardId != null) {
            cardIdFieldView.setText(cardId);
        } else {
            cardIdFieldView.setError(getString(R.string.noCardIdError));
        }

Since you requested these changes, let me know how you would like to proceed here.
Will the tests need to change or the logic?

[TheLastProject]

If the tests need changing after implementing the functionality because the tests turn out to not be correct but the functionality is, sure, change them.

However, from quick testing, I see some functionality is still not correct:

Creating a card without filling in name

1. Press +
2. Scan a barcode
3. Press checkmark to save

Expected: "Card name cannot be empty" warning comes from the red icon
Actual: "Card name cannot be empty" warning shows as a toast

Editing saved card

1. Open card
2. Press Edit

Expected: No warnings, because the card was saved so should be in a correct state
Actual: "Card ID" has a red warning which, when clicking, shows "No ID entered" despite an ID being there

It is logical that tests would fail if the behaviour is not correct yet. I'd recommend first fixing the behaviour before trying to change the tests, otherwise you may be changing tests into something that doesn't actually test the right thing.