Reviving previously expired sponsorships doesn't copy all data across OK

[jrosindell]

When we recover and already expired species back into reservations the 'username' field is not copied across as well.

This is strange because

if prev_row: for k in db.expired_reservations.fields: if (db.expired_reservations[k].writable and k in db.reservations.fields and r[k] is None and k not in fields_to_update): fields_to_update[k] = prev_row[k]

in

https://github.com/OneZoom/OZtree/blob/main/modules/sponsorship.py

should copy across all fields.

What I think is happening is that username is not set to "NULL" it's just a blank i.e. "" and so r[k] is None will evaluate as false and it won't be considered something to update

[jrosindell]

I can confirm after conducting a test that if I hand fill the db out with NULL in the username field of reservations then it works as expected on renewal. However, if it's left as blank (as currently) then it doesn't work

There is something about the code that creates a new entry in the reservations table that pre fills everything with NULL except for the username field. Maybe this is a web2py thing as it uses username for other things internally?

[lentinj]

Picking apart temp fix, the core of it is:

diff --git a/modules/sponsorship.py b/modules/sponsorship.py
index e83d8316..0f6f0944 100644
--- a/modules/sponsorship.py
+++ b/modules/sponsorship.py
@@ -350,6 +350,7 @@ def reservation_confirm_payment(basket_code, total_paid_pence, basket_fields):
                 # Renewal of expired entry, bump verified_time, keep old reserve time
                 fields_to_update['verified_time'] = request.now
                 fields_to_update['reserve_time'] = prev_row.reserve_time
+                fields_to_update["username"] = prev_row.username
                 if prev_row.partner_name:
                     fields_to_update['partner_name'] = prev_row.partner_name

diff --git a/tests/unit/test_modules_sponsorship.py b/tests/unit/test_modules_sponsorship.py
index 60515d3c..63359cfa 100644
--- a/tests/unit/test_modules_sponsorship.py
+++ b/tests/unit/test_modules_sponsorship.py
@@ -308,6 +308,7 @@ class TestSponsorship(unittest.TestCase):
             e_mail='001@unittest.example.com',
             user_sponsor_name="Arnold",  # NB: Have to at least set user_sponsor_name
             user_donor_name="Emily",
+            user_sponsor_kind="for",
             prev_reservation=None,
         ))
         reservation_confirm_payment('UT::BK001', 10000, dict(
@@ -315,10 +316,13 @@ class TestSponsorship(unittest.TestCase):
             PP_e_mail='paypal@unittest.example.com',
             sale_time='01:01:01 Jan 01, 2001 GMT',
         ))
-        util.verify_reservation(reservation_row, verified_name="Definitely Arnold")
+        username = util.verify_reservation(reservation_row, verified_name="Definitely Arnold")
         status, _, reservation_row, _ = get_reservation(ott, form_reservation_code="UT::002")
         self.assertEqual(status, 'sponsored')
+        self.assertEqual(reservation_row.verified_kind, "for")
         self.assertEqual(reservation_row.verified_donor_name, "Emily")
+        self.assertEqual(reservation_row.verified_name, "Definitely Arnold")
+        self.assertEqual(reservation_row.username, username)

         # Expire the reservation, won't be marked as renewed
         expired_r_id = reservation_expire(reservation_row)
@@ -358,6 +362,7 @@ class TestSponsorship(unittest.TestCase):
         self.assertEqual(reservation_row.e_mail, '002@unittest.example.com')
         self.assertEqual(reservation_row.PP_e_mail, None)
         self.assertEqual(reservation_row.PP_transaction_code, None)
+        self.assertEqual(reservation_row.username, None)

         # Buy it, compare details with expired row
         reservation_confirm_payment('UT::BK002', 10000, dict(
@@ -376,11 +381,14 @@ class TestSponsorship(unittest.TestCase):
         self.assertEqual(expired_r.sale_time, '01:01:01 Jan 01, 2001 GMT')
         self.assertEqual(expired_r.was_renewed, True)
         self.assertEqual(reservation_row.sale_time, '01:01:01 Jan 01, 2002 GMT')
+        self.assertEqual(reservation_row.username, username)
         self.assertEqual(reservation_row.user_sponsor_name, 'Arnold')
         self.assertEqual(reservation_row.verified_name, 'Definitely Arnold')
         self.assertEqual(reservation_row.PP_e_mail, 'paypal@unittest.example.com')
         self.assertEqual(reservation_row.PP_transaction_code, 'UT::PP2')
         self.assertEqual(reservation_row.verified_donor_name, "Emily")
+        self.assertEqual(reservation_row.user_sponsor_kind, "for")
+        self.assertEqual(reservation_row.verified_kind, "for")

     def test_reservation_confirm_payment__invalid(self):
         """Unknown baskets are an error"""
diff --git a/tests/unit/util.py b/tests/unit/util.py
index 4a8c550c..9b459761 100644
--- a/tests/unit/util.py
+++ b/tests/unit/util.py
@@ -254,7 +254,10 @@ def purchase_reservation(otts = 1, basket_details = None, paypal_details = None,

 def verify_reservation(reservation_row, **kwargs):
-    """Emulate verification logic in controllers/manage.py:SPONSOR_UPDATE"""
+    """
+    Emulate verification logic in controllers/manage.py:SPONSOR_UPDATE
+    and return the username
+    """
     # Add verified options
     reservation_row.update_record(
         verified_time=current.request.now,
@@ -269,3 +272,4 @@ def verify_reservation(reservation_row, **kwargs):
         username=username,
         **kwargs,
     )
+    return username

...which all makes sense, given the issue above. But I can't get the test to fail when added to main.

The default of "" could have easily come from web2py, as we clone it's definition of the username field https://github.com/web2py/web2py/blob/master/gluon/authapi.py#L358-L364, but this is something I can't replicate:

(Pdb) db.define_table('camel_requires', db.auth_user.username.clone(unique=False, requires=None))

mysql> SHOW CREATE TABLE camel_requires\G
*************************** 1. row ***************************
       Table: camel_requires
Create Table: CREATE TABLE `camel_requires` (
  `id` int NOT NULL AUTO_INCREMENT,
  `username` varchar(128) DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3
1 row in set (0.00 sec)

...but the DB schema won't necessarily line up with the code, especially when it's not an obvious problem.

[lentinj]

Right. So this is still technically a problem:

(Pdb) [x for x in db(db.reservations.id==db.reservations.insert(OTT_ID=5202546, name="Frank", num_views=1)).select(db.reservations.username)]
[<Row {'username': ''}>]

...but it's been masked by https://github.com/OneZoom/OZtree/commit/5f81922c8833a5cd12aa024dc2faf02ecc211599, which instead of removing the row, and letting get_reservations insert fresh copies, would NULL all fields of the existing row.

It's probably worth just forward-porting the fix anyway, given there could be existing entries with a blank username.

[lentinj]

@hyanwong once https://github.com/OneZoom/OZtree/pull/826 this is probably done. It might be worth trying to change the default for the username field to NULL, given PyDAL seems to do it's own thing rather than rely on the database, but we still potentially have to deal with old rows anyway.

[hyanwong]

Thanks @lentinj - sorry for the slow response: internet issues.

It should be easy enough to change the default to NULL. Are you suggesting this is done in db.py or directly in the database (or both)?

[lentinj]

Are you suggesting this is done in db.py or directly in the database

In db.py here:

https://github.com/OneZoom/OZtree/blob/fb96f14ff9802aaa25827f0aeb7e0180a450bb38/models/db.py#L463-L462

It seems like web2py does it's own thing for defaults, rather than relying on the database (the MySQL definition for reservations.username is DEFAULT NULL here), but I've not had a chance to confirm this.

[hyanwong]

I think I saw somewhere in the manual that web2py won't change the default on DB migration if the table already exists, but I think it will set a default if it needs to create a new table. So that could explain why they are out of sync?

[lentinj]

That's quite probably true, and that's why I was asking what the state of the live schema is earlier.

But on further poking, I don't think it's relevant. Here, MySQL says username` varchar(128) DEFAULT NULL, but db.reservations.insert() will still create an entry with username == "" by default. Web2Py (seems to be) setting it's own defaults in the SQL, rather than leaving them blank and letting the DB set it's default.

[hyanwong]

Hmm, weird. Perhaps somewhere the username field as defined in db.py is setting the default on insert()? It looks like we can set a default=XXX parameter when defining the field.

We currently say:

db.auth_user.username.clone(unique=False, requires=None),

but maybe that should be

db.auth_user.username.clone(unique=False, requires=None, default=None),

(or something like that: I'm not sure None is the correct thing here)

[lentinj]

but maybe that should be db.auth_user.username.clone(unique=False, requires=None, default=None),

Probably, but I'm not sure what clone does with the extra parameters. If it results in a correct default value on db.reservations.insert() and doesn't result in schema changes, then it's worth doing.

[hyanwong]

I guess I can hack that change (default=None) into db.py on beta and see if it causes a migration. If not, we're good to go.