search

elplatt / seltzer

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

BREAKING!! - install broken for dev branch when doing clean install; blocks #411 #413

Closed chris18890 closed 6 years ago

chris18890 commented 6 years ago

Blocks 0.5.6 release to master (#411)

Problem:

When doing a clean install of the dev branch, an error occurs during install

https://pastebin.com/QNmrjAmT

The error points to line 101 of the contact module, which upon further investigation is an SQL error being thrown. Tracing back further through the code the problem seems to be from line 76-105 if ($old_revision < 2) {; as part of the revision 2 module install code it's trying to create the contact_list permissions for the module, but it's failing because the DB table in question hasn't been created yet because the user_install function hasn't been run.

Possible solution:

Move this code from the contact module into the user module & increment the user & contact module revisions by 1.

I'll create a branch on my fork to test this & will try to have a PR (#414) ready by tonight

@elplatt @ramgarden for awareness

EDIT - credit @yommy from @somakeit for finding this!

elplatt commented 6 years ago

We really don't want to change any installation code unless we absolutely have to, because it could break things for people who are upgrading.

Which release did this stop working? And what changed?

On Fri, Mar 30, 2018 at 3:00 PM, Chris Murray notifications@github.com wrote:

Problem:

When doing a clean install of the dev branch, an error occurs during install

https://pastebin.com/QNmrjAmT

The error points to line 101 of the contact module, which upon further investigation is an SQL error being thrown. Tracing back further through the code the problem seems to be from line 76-105 if ($old_revision < 2) {; as part of the revision 2 module install code it's trying to create the permissions for the module, but it's failing because the DB table in question hasn't been created yet because the user_install function hasn't been run.

Possible solution:

Move this code from the contact module into the user module & increment the user & contact module revisions by 1.

I'll create a branch on my for to test this & will try to have a PR ready by tonight

@elplatt https://github.com/elplatt @ramgarden https://github.com/ramgarden for awareness

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/issues/413, or mute the thread https://github.com/notifications/unsubscribe-auth/AAS0WLfdaxATN6zvJuV_x-J_WHf_AW2vks5tjoDTgaJpZM4TCBmr .

-- Edward L. Platt https://elplatt.com | @elplatt | elplatt@social.coop

Tips for stopping email overload: https://hbr.org/2012/02/stop-email-overload-1

elplatt commented 6 years ago

Good catch though!

On Fri, Mar 30, 2018 at 3:50 PM, Edward L Platt ed@elplatt.com wrote:

We really don't want to change any installation code unless we absolutely have to, because it could break things for people who are upgrading.

Which release did this stop working? And what changed?

On Fri, Mar 30, 2018 at 3:00 PM, Chris Murray notifications@github.com wrote:

Problem:

When doing a clean install of the dev branch, an error occurs during install

https://pastebin.com/QNmrjAmT

The error points to line 101 of the contact module, which upon further investigation is an SQL error being thrown. Tracing back further through the code the problem seems to be from line 76-105 if ($old_revision < 2) {; as part of the revision 2 module install code it's trying to create the permissions for the module, but it's failing because the DB table in question hasn't been created yet because the user_install function hasn't been run.

Possible solution:

Move this code from the contact module into the user module & increment the user & contact module revisions by 1.

I'll create a branch on my for to test this & will try to have a PR ready by tonight

@elplatt https://github.com/elplatt @ramgarden https://github.com/ramgarden for awareness

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/issues/413, or mute the thread https://github.com/notifications/unsubscribe-auth/AAS0WLfdaxATN6zvJuV_x-J_WHf_AW2vks5tjoDTgaJpZM4TCBmr .

-- Edward L. Platt https://elplatt.com | @elplatt | elplatt@social.coop

Tips for stopping email overload: https://hbr.org/ 2012/02/stop-email-overload-1

-- Edward L. Platt https://elplatt.com | @elplatt | elplatt@social.coop

Tips for stopping email overload: https://hbr.org/2012/02/stop-email-overload-1

chris18890 commented 6 years ago

I think in this case we have to! =\ doing a clean install from the dev branch is totally broken. The code was written & tested from an upgrade POV, I mustn’t’ve factored in clean installs when testing otherwise I’d’ve picked it up then!

The 0.5.0 release (master) is grand but the 0.5.5 tag on dev is broken; I’ll look up the commit from the dev branch when on something larger than my phone, the network page doesn’t work on it!

elplatt commented 6 years ago

Let's figure out what went wrong first. We want to know what we're doing before changing anything, so we can make sure we're actually fixing the right thing.

On Fri, Mar 30, 2018 at 4:04 PM, Chris Murray notifications@github.com wrote:

I think in this case we have to! =\ doingna clean install from the dev branch is totally broken. The code was written & tested from an upgrade POV, I mustn’t’ve factored in clean installs when testing otherwise I’d’ve picked it up then!

The 0.5.0 release (master) is grand but the 0.5.5 tag on dev is broken; I’ll look up the commit from the dev branch when on something larger than my phone, the network page doesn’t work on it!

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

-- Edward L. Platt https://elplatt.com | @elplatt | elplatt@social.coop

Tips for stopping email overload: https://hbr.org/2012/02/stop-email-overload-1

chris18890 commented 6 years ago

I think I covered it in the original post - it was trying to create the contact_list permissions in the premissions table, but that table hadn’t been created yet, causing an error & the installation to fail.

elplatt commented 6 years ago

That's what's happening now, but why? Up until some release/commit, this code didn't break clean installs, so why is it breaking it now?

ramgarden commented 6 years ago

Thanks for finding and fixing this!

On Fri, Mar 30, 2018, 3:00 PM Chris Murray notifications@github.com wrote:

Problem:

When doing a clean install of the dev branch, an error occurs during install

https://pastebin.com/QNmrjAmT

The error points to line 101 of the contact module, which upon further investigation is an SQL error being thrown. Tracing back further through the code the problem seems to be from line 76-105 if ($old_revision < 2) {; as part of the revision 2 module install code it's trying to create the permissions for the module, but it's failing because the DB table in question hasn't been created yet because the user_install function hasn't been run.

Possible solution:

Move this code from the contact module into the user module & increment the user & contact module revisions by 1.

I'll create a branch on my for to test this & will try to have a PR ready by tonight

@elplatt https://github.com/elplatt @ramgarden https://github.com/ramgarden for awareness

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/issues/413, or mute the thread https://github.com/notifications/unsubscribe-auth/ACnBzUN1rY2S2lNd_qC3z8RvaVL0gVELks5tjoDTgaJpZM4TCBmr .

chris18890 commented 6 years ago

@elplatt I think it was commit 4eb884c340d1238bd22f2cb56d1caafb5e81a197 as part of PR #405 for release 0.5.3 > 0.5.4 that introduced the change, it clearly wasn't tested thoroughly enough with new installs, just upgrades

@ramgarden wasn't me, thank @yommy from @somakeit who found it the hard way when I gave them a dev instance to play with!

elplatt commented 6 years ago

Ah, I think I see why that broke it. Let me know if this is correct:

On Sat, Mar 31, 2018 at 1:50 PM, Chris Murray notifications@github.com wrote:

Reopened #413 https://github.com/elplatt/seltzer/issues/413.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elplatt/seltzer/issues/413#event-1550817590, or mute the thread https://github.com/notifications/unsubscribe-auth/AAS0WO4zJ9PJVBWQ1PDI6Sg3EotYvnurks5tj8H-gaJpZM4TCBmr .

-- Edward L. Platt https://elplatt.com | @elplatt | elplatt@social.coop

Tips for stopping email overload: https://hbr.org/2012/02/stop-email-overload-1

chris18890 commented 6 years ago

Yeah, pretty much, and the contact permissions were handled by the user module when it was installed. Will run the tests as listed in #414 to make sure that there are no scenarios where the app can’t be upgraded & leaves a dirty DB

elplatt commented 6 years ago

Sounds good!

On Sat, Mar 31, 2018 at 6:01 PM, Chris Murray notifications@github.com wrote:

Yeah, pretty much, and the contact permissions were handled by the user module when it was installed. Will run the tests as listed in #414 https://github.com/elplatt/seltzer/pull/414 to make sure that there are no scenarios where the app can’t be upgraded & leaves a dirty DB

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

-- Edward L. Platt https://elplatt.com | @elplatt | elplatt@social.coop

Tips for stopping email overload: https://hbr.org/2012/02/stop-email-overload-1

chris18890 commented 6 years ago

So at the moment, if a user does a clean install of dev, it fails because of errors discussed above; or if they go from (master >) dev > issue_413, they face errors, because the contact_list permission is now attempting to be added twice. Extra checks will need added to issue_413 to prevent this