fix: 'Bill Type: Invalid Choice: could not coerce' error

[rriski]

Error introduced in #1290. Fixes #1293. WTForms needs to be bumped to >=2.3.2
as it includes a fix to SelectField which is required for this change to work.

See:

• https://wtforms.readthedocs.io/en/3.1.x/changes/#version-2-3-2
• SelectField will throw ValueError when using list of strings pallets-eco/wtforms#598

[rriski]

There is a FormEnum helper here

ihatemoney/ihatemoney/utils.py

Lines 324 to 344 in 4af4c10

	class FormEnum(Enum):
	"""Extend builtin Enum class to be seamlessly compatible with WTForms"""
	@classmethod
	def choices(cls):
	return [(choice, choice.name) for choice in cls]
	@classmethod
	def coerce(cls, item):
	"""Coerce a str or int representation into an Enum object"""
	if isinstance(item, cls):
	return item
	# If item is not already a Enum object then it must be
	# a string or int corresponding to an ID (e.g. '0' or 1)
	# Either int() or cls() will correctly throw a TypeError if this
	# is not the case
	return cls(int(item))
	def __str__(self):
	return str(self.value)

but we can't use it since it expects int values for the extending class.

[TomRoussel]

Indeed I also found this while adding the enum, but I wanted to have an Enum with string values to easily show a "nice" string to the user.

[rriski]

Need item.upper() because the API expects that bill_type is passed as the enum value (i.e. "Expense" or "Reimbursement") but item is "EXPENSE" or "REIMBURSEMENT" when its passed as string to coerce.

[TomRoussel]

I've been playing around with the changes in this PR. To me it seems like BillType.coerce in unnecessary. We can just use the enum constructor for this. BillType("Expense") correctly returns BillType.Expense.

I don't really understand why, but I think the key change that fixes the error in the form is adding BillType.__str__?

[TomRoussel]

I quickly pushed my change to explain what I mean: See this commit.

When I run the server with this change, I also don't get the error message.

[TomRoussel]

Okay, I figured out what's going on. The WTForms SelectField sends the different choices to the browser as strings (documented here. These are specified by BillType.choices() as a list of (value, label) tuples. The value should be the actual enum instance, and label would then be what the user sees. Since WTForms encodes everything as a string, it also sends this value to the browser as str(value). For an ordinary enum, this becomes "BillType.EXPENSE" for example. When the browser then sends the post request, it uses that string, but that's not understood by the coerce function.

That's why adding an explicit BillType.__str__ fixes the problem. It's actually the equivalent as modifying choices as follows:

    @classmethod
    def choices(cls):
        return [(choice.value, choice.value) for choice in cls]

[rriski]

Okay, I figured out what's going on. The WTForms SelectField sends the different choices to the browser as strings (documented here. These are specified by BillType.choices() as a list of (value, label) tuples. The value should be the actual enum instance, and label would then be what the user sees. Since WTForms encodes everything as a string, it also sends this value to the browser as str(value). For an ordinary enum, this becomes "BillType.EXPENSE" for example. When the browser then sends the post request, it uses that string, but that's not understood by the coerce function.

That's why adding an explicit BillType.__str__ fixes the problem. It's actually the equivalent as modifying choices as follows:

    @classmethod
    def choices(cls):
        return [(choice.value, choice.value) for choice in cls]

Thanks, I have updated the PR to use:

@classmethod
    def choices(cls):
        return [(choice.value, choice.value) for choice in cls]

[TomRoussel]

That looks good to me! Ah nice, you also caught the duplicate tests, I just saw those today.

[zorun]

Thanks for the fix.

Our CI was failing, which means that unformatted code went through in the last PR. I have fixed the issue in #1296 and hopefully reorganized the CI to avoid this in the future.

It does mean that you have to rebase and possibly fix conflicts, sorry about that. But we should then get a clearer view of the CI.

[zorun]

The testcases with the lower versions for dependency are failing with HTTP 400 errors. It means that the approach doesn't work with older version of some libraries (here, most probably wtforms)

Does your solution rely on a undocumented behaviour in wtforms? If so, it's preferable to find another solution that will be less fragile over time.

If there is really no other workable approach, you can try bumping the minimum version of wtforms.

[TomRoussel]

Very strange, as far as I can tell, this solution is not using anything exotic from wtforms. All api calls are following the documented way of working for 2.3.x of wtforms, which should correspond to the minimal version. What I think is strange is that this test is breaking now, but the minimal version checks in #1290 were fine.

I can try replicating the problem when I have time later this week.

[TomRoussel]

I found the culprit I think. I believe the problem is related to a bug fixed in a PR of WTForms here. The changelog for 2.3.2 mentions that they fixed a bug in the SelectField.choices field. Using WTForms 2.3.1 fails, but 2.3.2 succeeds. Reading the PR description, I'm really not sure why our implementation here doesn't work. The only reason I can think of is that their validation logic for 2.3.1 somehow breaks if the choices tuples are identical values?

Is see 2 solutions to this problem, either we do a minor version bump for WTForms from 2.3.1 --> 2.3.2, or we use the following workaround which also seems to do the trick:

class BillType(Enum):
    EXPENSE = "Expense"
    REIMBURSEMENT = "Reimbursement"

    def __str__(self):
        return self.value

    @classmethod
    def choices(cls):
        return [(choice, choice.value) for choice in cls]

Using a BillType instance as the first member of the choice tuple seems to not trigger the validation bug, while the str method makes sure flasks sends a sensible value to the browser which can then be coerced back to the enum.

By the way, sorry @rriski that you got involved in this weird can of worms from my implementation!

[zorun]

Thanks for looking at it!

If it's a bug in wtforms, then let's use the simplest solution: @rriski , can you bump the wtforms dependency to >=2.3.3 ?

[rriski]

Thanks for looking at it!

If it's a bug in wtforms, then let's use the simplest solution: @rriski , can you bump the wtforms dependency to >=2.3.3 ?

Sure, updated PR now. Thanks @TomRoussel for figuring out the issue.