'was_renewed' field set incorrectly in expired reservations.

[jrosindell]

'was_renewed' field set incorrectly in expired reservations.

[hyanwong]

Anyway, panic over. We thought these had all gone missing completely! But it's all recoverable, phew.

[hyanwong]

Needs a test writing, I guess.Have you looked for "was_renewed" in the test suite?

[lentinj]

Yeah, this is fairly obviously problematic:

OZtree/modules/sponsorship.py

Lines 167 to 171 in 59720b8

	expired_r = r.as_dict()
	del expired_r['id']
	expired_r['was_renewed'] = True
	expired_id = db.expired_reservations.insert(**expired_r)
	r.delete_record()

IIRC we use this function both in expiry and in renewal. We only need to do this bit in the latter.

[hyanwong]

Thanks @lentinj - Those lines are indeed part of the problem. I've been chatting about this with James. We think there are 2 things wrong:

1. "was_renewed" in the expired_reservations table is always set to True when expiring, even when the sponsorship wasn't renewed, simply expired
2. The row in the main reservations table should not be deleted on expiry: instead, the user-specific fields should be wiped, but some elements should stay the same (e.g. the visit count). I think we simply didn't make it clear this was required when asking you to code it up.

[lentinj]

Yeah (1) is pretty obvious how to sort, and wouldn't take very long.

And yes, I'd not realised about (2). Do we know what fields should be preserved, besides num_views?

[hyanwong]

Yeah (1) is pretty obvious how to sort, and wouldn't take very long.

Great

And yes, I'd not realised about (2). Do we know what fields should be preserved, besides num_views?

id, OTT_ID, name, num_views, last_view, and deactivated should do it.

[lentinj]

Is was_renewed True for a expire-and-repurchase as well as a renewal? I'm assuming yes.

[lentinj]

Gah, somehow I managed to not notice the convenient clear_reservation() just above that does exactly what we want.

OZtree/modules/sponsorship.py

Lines 138 to 144 in 59720b8

	def clear_reservation(reservations_table_id):
	db = current.db
	keep_fields = ('id', 'OTT_ID', 'num_views', 'last_view')
	del_fields = {f: None for f in db.reservations.fields if f not in keep_fields}
	assert len(keep_fields) + len(del_fields) == len(db.reservations.fields)
	assert reservations_table_id is not None
	db(db.reservations.OTT_ID == reservations_table_id).update(**del_fields)

(even more annoyingly, I was the one that moved it there in the first place. Ah well.)

But, it doesn't preserve name or deactivated. is that a bug? Former is probably not that much of a deal either way, the latter might be.

[lentinj]

Either way, the code changes are above. Note the latter also changes get_reservation to return up-to-date counts with it's reservation row. This shouldn't make a difference in real life since we don't do anything with the counts, but potentially reduces unit-test confusion in future.

[lentinj]

@hyanwong The above commit removes as much of the deactivated field as possible without restructuring the database. It's not part of the pull request above but can add it if you still think it's a good plan?