search

elplatt / seltzer

CRM for hackerspaces
GNU General Public License v3.0
104 stars 50 forks source link

set values to be non-null #425 #211 #427

Closed chris18890 closed 5 years ago

chris18890 commented 5 years ago

set values to be non-null to get around error shown in #425 & #211

chris18890 commented 5 years ago

@elplatt @ramgarden can I get a review on this?

chris18890 commented 5 years ago

Not sure, the first cid is 1 which is the admin user, all subsequent ones are positive intergers

ramgarden commented 5 years ago

Can we absolutely guarantee that there will never be a contact with ID of zero? If so then it's ok. If we can't then it should be -1. What data type is that field? Can it even hold a negative number?

chris18890 commented 5 years ago

No we can't, though the table code in /crm/include/sys/module.inc.php line 272-281 starts at 1 & increments, & the DB fields are set to mediumint(8) unsigned NOT NULL AUTO_INCREMENT, will need to see if it can hold a negative number

ramgarden commented 5 years ago

I think the unsigned means we can't put negative numbers in that field. Zero might be our only option. If it sets the initial admin user ID to 1 then auto_increments we should be ok with zero as CID values.

chris18890 commented 5 years ago

@ramgarden correct!

Unsigned values can be used when you want to allow only nonnegative numbers in a column and you need a larger upper numeric range for the column. For example, if an INT column is UNSIGNED , the size of the column's range is the same but its endpoints shift from -2147483648 and 2147483647 up to 0 and 4294967295 .

from https://beta-reduction.com/mysql/doc/refman/5.1/en/numeric-types.html

My code is probably quite hacky, there's probably a better way, but I think it'd basically involve making a DB schema change to remove the "not null" attribute on the columns, so I dunno if we want to do that! Will let @elplatt have the final say:)

ramgarden commented 5 years ago

If we are having to set values for attributes in a row we insert just to make it work, it sounds like those attributes should allow nulls, right? Since we are basically treating them as null / empty data by setting them to zero. Is there any other code that might break if we change the schema to allow nulls on those attributes?

On Thu, Apr 4, 2019 at 2:14 PM Chris Murray notifications@github.com wrote:

@ramgarden https://github.com/ramgarden correct!

Unsigned values can be used when you want to allow only nonnegative numbers in a column and you need a larger upper numeric range for the column. For example, if an INT column is UNSIGNED , the size of the column's range is the same but its endpoints shift from -2147483648 and 2147483647 up to 0 and 4294967295 .

from https://beta-reduction.com/mysql/doc/refman/5.1/en/numeric-types.html

My code is probably quite hacky, there's probably a better way, but I think it'd basically involve making a DB schema change to remove the "not null" attribute on the columns, so I dunno if we want to do that! Will let @elplatt https://github.com/elplatt have the final say:)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/pull/427#issuecomment-480006633, or mute the thread https://github.com/notifications/unsubscribe-auth/ACnBzTOpFD3UapMBPpjMiV5KNZMgWBcBks5vdkD4gaJpZM4cbs27 .

chris18890 commented 5 years ago

Agreed, and for new setups it should be relatively easy to do so, but it'll require a review of all modules' install/DB table creation functions to ensure consistency & that it's actually (not) set as needed, then making sure we can modify the existing tables with a SQL command to avoid major breakages!

chris18890 commented 5 years ago

And is it better to merge this in now as a fix & circle back later to do a review, or start making the DB changes now?

ramgarden commented 5 years ago

I would say it's OK to merge in now but make sure to note that it's a temporary fix until we fix the schema to allow nulls in those specific fields.

On Thu, Apr 4, 2019 at 2:23 PM Chris Murray notifications@github.com wrote:

And is it better to merge this in now as a fix & circle back later to do a review, or start making the DB changes now?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/pull/427#issuecomment-480009935, or mute the thread https://github.com/notifications/unsubscribe-auth/ACnBzVYaL0-hvAIYfyzk3_4vpofJNAg1ks5vdkMfgaJpZM4cbs27 .