Closed
Bug 674316
Opened 14 years ago
Closed 14 years ago
Account portal stage bug creating a user - lib handles integer badly
Categories
(Cloud Services :: Server: Core, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: telliott, Assigned: telliott)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
1.38 KB,
patch
|
rmiller
:
review+
|
Details | Diff | Splinter Review |
Giving a value of 1 to accountStatus bombs out, even though it's defined in the schema as an int. making it '1' solves the problem.
Also cleaning up a couple small errors that we found while on stage.
Attachment #548546 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #548546 -
Flags: review? → review?(rmiller)
Assignee | ||
Comment 1•14 years ago
|
||
Added a little bit I didn't mean to. Pulling that out
Assignee: nobody → telliott
Attachment #548546 -
Attachment is obsolete: true
Attachment #548546 -
Flags: review?(rmiller)
Attachment #548549 -
Flags: review?(rmiller)
Comment 2•14 years ago
|
||
Comment on attachment 548549 [details] [diff] [review]
revised version to clean up the problems
Review of attachment 548549 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #548549 -
Flags: review?(rmiller) → review+
Comment 3•14 years ago
|
||
1/ there's an API in the ldap lib to do this in fact. It's not used right now but we should use it. It converts all values to strings for you
2/ if it bombs out, are we actually catching that error properly ?
3/ why are you adding **kw in this patch ?
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #3)
> 1/ there's an API in the ldap lib to do this in fact. It's not used right
> now but we should use it. It converts all values to strings for you
>
OK, we can do that in the future.
> 2/ if it bombs out, are we actually catching that error properly ?
>
yes. The problem is that the test wasn't running properly before and I hadn't realized it. Once I got it to actually test this, it fails.
> 3/ why are you adding **kw in this patch ?
It's another bugfix I caught when I got the tests to run. Which I'm pretty sure that we discussed and approved before elsewhere and can dig out once I get this to atoll.
Comment 5•14 years ago
|
||
Comment 6•14 years ago
|
||
(In reply to comment #4)
> OK, we can do that in the future.
Why should we postpone a better future-proof fix ?
>
> > 2/ if it bombs out, are we actually catching that error properly ?
> >
>
> yes. The problem is that the test wasn't running properly before and I
> hadn't realized it. Once I got it to actually test this, it fails.
>
> > 3/ why are you adding **kw in this patch ?
>
> It's another bugfix I caught when I got the tests to run. Which I'm pretty
> sure that we discussed and approved before elsewhere and can dig out once I
> get this to atoll.
This sounds like another issue to me
Assignee | ||
Comment 7•14 years ago
|
||
(In reply to comment #6)
> (In reply to comment #4)
> > OK, we can do that in the future.
>
> Why should we postpone a better future-proof fix ?
>
We shouldn't. We'll do it, possibly very quickly. But the current solution does work.
> > > 3/ why are you adding **kw in this patch ?
> >
> > It's another bugfix I caught when I got the tests to run. Which I'm pretty
> > sure that we discussed and approved before elsewhere and can dig out once I
> > get this to atoll.
>
> This sounds like another issue to me
You are correct. Mea culpa, I was trying to get this one out asap. I'll file a separate bug for that issue.
Assignee | ||
Comment 8•14 years ago
|
||
(In reply to comment #7)
> > This sounds like another issue to me
>
> You are correct. Mea culpa, I was trying to get this one out asap. I'll file
> a separate bug for that issue.
Filed Bug 674316 to track this one
Assignee | ||
Comment 9•14 years ago
|
||
Changes made in http://hg.mozilla.org/services/server-core/file/1ab5fb15ba0e
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•14 years ago
|
||
(In reply to comment #8)
> Filed Bug 674316 to track this one
Oops. That's Bug 674347
Also filed Bug 674414 for the ldap modlist issue.
Comment 11•14 years ago
|
||
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > OK, we can do that in the future.
> >
> > Why should we postpone a better future-proof fix ?
> >
>
> We shouldn't. We'll do it, possibly very quickly. But the current solution
> does work.
It works, but that does not explain why you did not apply the better solution right away instead of duplicating the workload, since it was not a hotfix.
Updated•13 years ago
|
Status: RESOLVED → VERIFIED
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•