Fixing errors from testing #53
Conversation
Pulling in upstream changes
|
Hmm, these are a little strange for two separate reasons:
The strange thing about all of these is that I can't find any of them in our master branch (i.e.,
Maybe it doesn't matter, but it makes me a little nervous that all these things unaccountably went wrong... Is there a handy way to see which commits introduced the bad lines and I can check to see if they're in any of our branches (or maybe you can check this all on your end with superior PR-fu)?
This one is weird because we've been using all of this code for quite a while and haven't noticed any problems with the checkboxes (which sweet jeebus I hope we would notice). How were they going wrong exactly? |
|
Hrm, I think this mainly went wrong in PR #34. I have introduced another I'm not sure about item 4, however. I didn't see anything which changed that. |
Okay so the extra The freeform stuff seems more likely to have been something you introduced for your project maybe? Since I don't think we've ever had any freeform fields...
What was the buggy behavior? Testing both our master and pure-upstream branch just now it seemed to me like checkboxes were working as expected... |
It could explain this, yes. Not sure where the freeform stuff is from, but it wasn't actually implemented anywhere, I don't think.
It would throw a 400 error (Bad Request). Technically, if you merged in the change I made to the controller, it shouldn't change anything but just enforce a null for the text. |
So the coder would see the 400 every time any checkbox changed? If so then we definitely would have noticed that and definitely haven't been seeing that behavior. So uh... yeah I remain puzzled/paranoid about why that would be happening now.
Hmh. The bugs are in MPEDS:master already? If so then I guess I can pull it and have a look at the git-blames just to assuage my doubt huh |
|
Yeah... idk. Pulling now. |
Okay so this one is from 03a1633, wherein it looks like I tried to reset some templates to the versions your team was using after cavalierly changing them. My guess is that I restored an older version. So yeah this one makes sense (and fix it however works). On to the next one. I actually kind of enjoy |
Okay yeah I can confirm that this one happened in today's merges (not sure where exactly since I don't want to bisect two changes, but |
|
And you said you did the That leaves the checkbox thing, which I'd like to understand so I can confirm we haven't been losing a bunch of checkbox data for months. Lemme see if I can replicate the behavior from a few commits ago... |
|
Hmm @alexhanna do you know a commit where the page would load at all but the checkboxes were broken? |
|
I don't. 🙃 I'm not sure what could have pushed that change. If it's helpful, you could look around changes to line 280 in |
|
Hah, okay. But there should be such a commit right? Like it didn't only happen with some uncommitted changes or something? |
|
That would make sense in theory. My worry is that there was some systems update (not sure, like in Python versioning or JavaScript) which broke it. |
|
But those didn't happen today, right? So there should be some commit from today that I can checkout and load the coding page and replicate the 400? |
|
Hypothetically, yes? If you look at the last commit before this PR, right? |
Yeah, but that one gives me a 500 when I try to load anything. Starting a little earlier, I managed to get the 400, which I seem to have introduced almost exactly a year ago in 651b368. But I can't seem to find how it got fixed in our version and not in yours. I guess it's possible that I fixed it in a branch I haven't PR'd (yet?) and then did a lousy job of packaging the PRs. I'll have another stab at finding when the fix went in, but this does make me feel better that we haven't been losing any data since this error was obvious enough not to have lasted long... But also it'd be nice to see how it got fixed in the first place. |
|
Ah, weird. Why does it give you a 500? What does the Apache log say? Do you see how it got fixed in my last PR? Did you make a similar change in your branches some place? Thanks for hunting this down, regardless. |
|
Okay, got it: the fix is at 3ec2d88, which makes sense -- I remember that business about changing the field from required to optional. So that fix might be better than the new one I think? But now it'd be nice to track down why that fix got overwritten today... I think the 500 was from a DB config problem btw, so I had to find a commit that had one bug and not the other. |
|
Ah yeah I like that fix more. Do you want me to push it? |
|
Yeah go for it. I'm not sure how it wasn't in there already, but I guess it doesn't matter much now that we know how to fix it? |
|
All right, done in #54. |
|
Okay cool. It's really nagging at me now why that fix wasn't in even though the commit with it was, but I'm out of debugging time for tonight. Maybe tomorrow I'll either figure it out or realize why it doesn't matter. Thanks for all the PR work today btw! Exciting times |
|
Very exciting. Maybe next time it won't take me a literal year to get to PRs. |
|
Okay I think I see what happened with the checkboxes. It looks they actually got merged in properly at 663249d but then immediately got clobbered back by the merge at 7562711, which was a branch that predated the fix. I'm guessing this would have created a merge conflict, but it would have been awfully tricky to spot which version was correct without looking at the commit dates (and maybe even then?). There is also a difference between those two branches on whether the Version control is hard, huh. I'm not sure how we could have done this to avoid this snafu aside from doing the PRs closer to when they were written so that we kept the temporal order and so they were fresher in our minds... Anyway I think these are all now explained and fixed, so I'll close this. It does make me wonder though whether I should have a quick check to make sure that the functionality of each accepted branch has indeed made it in... |
Whoops already closed I guess. I need to lrn2PR better heh |
|
Thanks for sleuthing this. I think having stricter PR discipline is important as well... e.g. accepting things in order (which I didn't). Eeep. |
|
Yeah I blame past-me, who had even less git skillz than present-me |
|
At least you sleuthed it. Current-me would have ignored it. Absolute git-newb. |
Four changes I discovered after testing. @davidskalinder -- do you want to review these?
models.py. Not sure why this was removed._del_eventfunction inmpeds_coder.py.actors-freeformback topersons-freeform. Not sure why this was changed.POSTso I added anullparameter and added some controller code to handle this.