sethblack / python-gift-exchange

pyge: Holiday Gift Exchange Picker
Apache License 2.0
250 stars 13 forks source link

Changing any variables of a User nullifies their history (I think) #4

Open DreamShark-Bytes opened 1 year ago

DreamShark-Bytes commented 1 year ago

Sorry but I couldn't get the code to run so I can't test this (probably just a result of my ignorance on all this, still super green when it comes to GitHub), but in my attempt to learn from your code, I think I found a potential bug.

I'm seeing that when it performs save_history, it's using the custom hash function you laid out in the Person class, which is this:

    def __hash__(self):
        return int(hashlib.md5(
            f'{self.name}:{self.dob}:{self.sex}:{self.lat_lng}'.encode('utf-8')
        ).hexdigest()[:8], 16)

Does this combine all of those fields (name, dob, sex, and location) into a unique identifier? Meaning, any changes to those items would break the user's link to their gift exchange history? I feel like name and location are especially susceptible to change given that the scope your wife gave you was "3 years" of history. The people could move locations, change their names from getting married, or change genders/pronouns.

All this means a person with a change could more easily be assigned someone they've been assigned to previously.

here was my attempt to re-create the bug: possible example of bug

sethblack commented 1 year ago

It does! Good catch! Especially on the gender/pronouns, names, and locations; totally legit. The hashing algo mixes everything.

Hmm. 🤔 I'll have to give this one some thought, but I think I can fix it.

cwkingjr commented 1 year ago

It seems like the simplest fix is to require unique names (which is likely an implicit requirement anyhow). However, to cover all bases, you'd still have to do something like include a history updater routine that accepts old and new names and rebuilds the history hashes.