#343 Added badges in "people.xsd" #346
Conversation
|
Job #346 is now in scope, role is |
|
This pull request #346 is assigned to @amihaiemil/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @g4s8/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be a monetary reward for this job |
|
@amihaiemil ping |
| </xs:sequence> | ||
| <xs:attribute name="updated" type="xs:dateTime" use="required"/> | ||
| </xs:complexType> | ||
| <xs:unique name="personBadge"> |
There was a problem hiding this comment.
@carlosmiranda You added this unique constraint, which seems ok in itself but given 0crat's "stateless" manner of work, won't it cause crashes? The way 0crat is working, I expect the following behaviour:
-
User reaches 1024, gets the "expert" badge, which will remain, no matter how the reputation fluctuates (I remember that was the idea behind badges).
-
User goes bellow 1024
-
User reaches 1024 again and 0crat, being as dumb as it is, will try to add the "expert" badge again.
What do you think?
There was a problem hiding this comment.
@amihaiemil well, if we don't make it unique, then we will eventually have multiple master (et. al) badges. Do you have an alternate suggestion?
There was a problem hiding this comment.
@carlosmiranda The fact that they are unique is OK. We have a problem only if the behaviour of 0crat will be as I described in my comment.
But, on the other hand, if that happens, it's a bug with the groovy/Java code in /farm (which should check if the badge already exists or not). Let's leave it like this.
| @@ -84,6 +84,26 @@ SOFTWARE. | |||
| <xs:field xpath="."/> | |||
| </xs:unique> | |||
| </xs:element> | |||
| <xs:element name="badges" minOccurs="0"> | |||
There was a problem hiding this comment.
@carlosmiranda I guess because you added minOccurs="0" here, we do not need an upgrate, right?
There was a problem hiding this comment.
@amihaiemil that's the idea. Not sure this is the preferred approach but I wanted to avoid making the element mandatory. Besides, the user doesn't have badges until he reaches 1024 points.
|
@amihaiemil replied |
|
@rultor good to merge |
@amihaiemil Thanks for your request. @yegor256 Please confirm this. |
|
@g4s8 I guess you are also the ARC on |
|
@rultor merge |
|
@g4s8 I think you should edit |
|
@g4s8 ping |
|
@amihaiemil done: #349 |
|
@g4s8 great, now let's wait for Yegor to merge that? :)) |
|
@amihaiemil/z this job was assigned to you 5days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please. |
|
@yegor256 ping, this is currently held up because Rultor is waiting for you :) |
|
@g4s8 Aren't you able to merge manually? |
|
@g4s8 ping |
|
@rultor merge |
|
@ypshenychka/z please review this job completed by @amihaiemil/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #346 is now out of scope |
|
@amihaiemil According to our QA Rules:
Only two issues were found during code review. |
|
@ypshenychka confirmed :D |
|
@amihaiemil Thanks |
|
@0crat quality acceptable |
|
Order was finished, quality is "acceptable": +15 point(s) just awarded to @amihaiemil/z |
|
Quality review completed: +8 point(s) just awarded to @ypshenychka/z |
#343: Added badges to
people.xsd. Added tests for both positive and negative cases.