View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0001065Main CAcert Websiteweb of trustpublic2012-06-02 09:272013-11-26 23:10
ReporterINOPIAE Assigned ToNEOatNHNG  
PrioritynormalSeverityminorReproducibilityhave not tried
Status needs workResolutionopen 
Product Version2013 Q2 
Summary0001065: Wrong wording when sending mails during the assurance process
DescriptionThe mail confirming the assurance show the following:

You issued 35 points and they now have 35 points in total.

This should be either

You issued 35 points and he/she now has 35 points in total.
or
You issued 35 points and John Doe now has 35 points in total.
line 359 wot.php


Line 357 wot.php
You issued 35 points however the system has rounded this down to 0 and they now have 150 points in total.

New:
You issued 35 points however the system will only count 100 assurance points and 50 experience points.

Line 325 wot.php
You were issued 35 points however the system has rounded this down to 0 and you now have 150 points in total.

New:
You were issued 35 points however however the system will only count 100 assurance points and 50 experience points.
TagsNo tags attached.
Reviewed by
Test Instructions

Relationships

related to 0001050 needs review & testingBenBE Review the code regarding the new point calculation in ./www/wot.php 

Activities

INOPIAE

2013-05-01 11:27

updater   ~0003951

pushed fix to https://github.com/INOPIAE/CAcert/commit/a1b7678c4ad46aae4c3e647bfbef173dd2f0695c

Werner Dworak

2013-05-01 16:05

updater   ~0003966

Last edited: 2013-05-01 16:08

 The text looks good for assurer and assuree --> ok

JensK

2013-05-06 13:04

reporter   ~0003974

Last edited: 2013-05-06 13:05

 Got two languages in one email in one case:

"You are receiving this email because you have been assured by .

Ihnen wurden 10 Punkte vergeben. Sie haben nun insgesamt 70 Punkte."

Otherwise, the wordings look as described.

However, using the word "however" without any commas around it sounds very strange to me. May I suggest using "but" instead of "however":

"You issued 35 points, but the system will only count..."

MartinGummi

2013-06-15 09:18

updater   ~0004058

You issued 10 points however the system only counts up to 100 assurance points. User Name has now 100 countable assurance points and 100 countable expierence points.

=> fail

MartinGummi

2013-06-15 09:20

updater   ~0004059

Reminder sent to: MartinGummi

re test

Eva

2013-09-10 21:16

updater   ~0004304

Last edited: 2013-09-10 21:17

 I assured 317,seo13@acme.com with 0 points and got the following mail:

"You are receiving this email because you have assured 317 sep13 (317.sep13@acme.com).

You issued 35 points and 317 sep13 now has 35 points in total.[...]"

-> ok


I assured KatziTest1@cacert.org who had 70 points before.

The mail of the Assuree is:
"You are receiving this email because you have been assured by Admin Katzi (KatziAdmin@cacert.org).

Ihnen wurden 35 Punkte vergeben. Sie haben nun insgesamt 100 Punkte.

Sie haben mindestens 100 Assurancepunkte erhalten. Sie können versuchen die CAcert Assurer Challenge zu bestehen, um CAcert Assurer zu werden. ( https://cats.cacert.org ) [...]"

-> No rounding down or however part mentioned

The mail the Assurer got:
"You are receiving this email because you have assured Test1 Katzi (KatziTest1@cacert.org).

You issued 35 points and Test1 Katzi now has 100 points in total. [...]"

-> No rounding down or however part mentioned

I'm not sure what the intended wording is, but this looks as a fine way to do it. (It's not relevant for anybody if a rounding took place, especially not for the assurer.)

NEOatNHNG

2013-09-10 22:48

administrator   ~0004307

I have added a few changes. Please test and review.

Eva

2013-09-10 23:18

updater   ~0004309

Last edited: 2013-09-24 22:45

 I assured an acc with 70 points before the assurance.

The assurer got the following mail:
"You are receiving this email because you have assured Test1 Katzi (KatziTest1@cacert.org).

You issued 35 points.

Best regards,
CAcert Support Team"

-> no mention of the total points of the assuree or the rounding (which is consistent)

The assuree got the following mail:
"You are receiving this email because you have been assured by Admin Katzi (KatziAdmin@cacert.org).

You were issued 35 points. However the system only counts up to 100 assurance points.

Sie haben mindestens 100 Assurancepunkte erhalten. Sie können versuchen die CAcert Assurer Challenge zu bestehen, um CAcert Assurer zu werden. ( https://cats.cacert.org )
[...]"

-> the rounding down is not mentioned, the information for max-points is there

=> ok

BenBE

2013-09-21 05:34

updater   ~0004343

The if-Conditions in the changed locations should use block markers {} for better source formatting. Apart from this the patch looks reasonable.

Regarding the wording I don't think you need to tell the assurer how many experience points the assuree already got - if you really wanted to close the information leakage.

BenBE

2013-10-23 19:19

updater   ~0004411

Regarding current source there's some weird code I don't think is right. Let's consider:

---
@@ -249,23 +249,24 @@ $iecho= "c";
        if($oldid == 6)
        {
                $max = maxpoints();
 
                $awarded = $newpoints = intval($_POST['points']);
                if($newpoints > $max)
                        $newpoints = $awarded = $max;
                if($newpoints < 0)
                        $newpoints = $awarded = 0;
 
                $query = "select sum(`points`) as `total` from `notary` where `to`='".$_SESSION['_config']['notarise']['id']."' group by `to`";
                $res = mysql_query($query);
                $drow = mysql_fetch_assoc($res);
+ $oldpoints = intval($drow['total']);
 
                $_POST['expire'] = 0;
 
- if(($drow['total'] + $newpoints) > 100 && $max < 100)
- $newpoints = 100 - $drow['total'];
- if(($drow['total'] + $newpoints) > $max && $max >= 100)
- $newpoints = $max - $drow['total'];
+ if(($oldpoints + $newpoints) > 100 && $max < 100)
+ $newpoints = 100 - $oldpoints;
+ if(($oldpoints + $newpoints) > $max && $max >= 100)
+ $newpoints = $max - $oldpoints;
                if($newpoints < 0)
                        $newpoints = 0;
 
---

The second group of additions doesn't make much send and is hard to read. Wouldn't it be better to have something like this?

// Get number of new points, based on points awarded and limit to at most 100 Points.
$newpoints = min($oldpoints + $awarded, 100);
// Retrieve number of actually awarded points, and ensure at least 0 as lower bound
$newpoints = max($newpoints - $oldpoints, 0);

Much more readable and at least not as utterly broken as the current code.

Simular could be done for the $max variable at the top (simular to above):
$awarded = $newpoints = min($max, $awarded);
$awarded = $newpoints = max(0, $awarded);

Review NACK.

Uli60

2013-10-24 13:45

updater   ~0004412

assurances given

------------------------------------------------------------------------------
You are receiving this email because you have assured xyz (xyz email).
You issued 10 points.
------------------------------------------------------------------------------

------------------------------------------------------------------------------
You are receiving this email because you have assured xyz (xyz email).
You issued 35 points.
------------------------------------------------------------------------------


assurance received

------------------------------------------------------------------------------
You are receiving this email because you have been assured by xyz (xyz email).
You were issued 35 points and you now have 35 points in total.
------------------------------------------------------------------------------


------------------------------------------------------------------------------
You are receiving this email because you have been assured by xyz (xyz email).
You were issued 35 points and you now have 80 points in total.
You now have over 50 points, and can now have your name added to client certificates,
 and issue server certificates for up to 2 years.
------------------------------------------------------------------------------


------------------------------------------------------------------------------
You are receiving this email because you have been assured by xyz (xyz email).

You were issued 35 points. However the system only counts up to 100 assurance points.

You have at least 100 Assurance Points, if you want to become an assurer try the Assurer Challenge ( https://cats.cacert.org )

To make it easier for others in your area to find you, it's helpful to list yourself as an assurer (this is voluntary), as well as a physical location where you live or work the most. You can flag your account to be listed, and add a comment to the display by going to:
https://www.cacert.org/wot.php?id=8

You can list your location by going to:
https://www.cacert.org/wot.php?id=13
------------------------------------------------------------------------------

checked F2F -> ok
checked TTP -> problems, see bug 1216

Uli60

2013-10-24 14:11

updater   ~0004413

Last edited: 2013-10-24 14:15

 comment on https://bugs.cacert.org/view.php?id=1065#c4411
$awarded = $newpoints = min($max, $awarded);
$awarded = $newpoints = max(0, $awarded);
limits the boundaries minimum 0 points, maximum the assurer can award
this makes it to awarded and newpoints
this is clarified and ok
the previous code in at least following line is wrong:
if(($oldpoints + $newpoints) > $max && $max >= 100)
oldpoints is current received points of an assuree eg 80 AP
now we add 35 new points
I do not need to add 35 points as 80 AP is still over maximum max !
and max (35 points) never can reach the && part max >= 100

but doesn't have the two blocks to be switched around?
first check awarded/points to their boundaries
then check the awarded/new points with the assurances the assuree
still received ?

the 2 lines then makes the final assurance:
$newpoints = min($oldpoints + $awarded, 100);
$newpoints = max($newpoints - $oldpoints, 0);

Awarded is still the value the assurer enters wether the fact the assuree has enough points or not. The validity range check of awarded depends on experience points the assurer received. The newpoints may be adjusted by that, but not the awarded points.

?!? newpoints is assigned twice, first in awarded verification section, later in the newpoints calculation

lets simplify =:)

$awarded = min($max, $awarded);
$awarded = max(0, $awarded);
$newpoints = min($oldpoints + $awarded, 100);
$newpoints = max($newpoints - $oldpoints, 0);

Issue History

Date Modified Username Field Change
2012-06-02 09:27 INOPIAE New Issue
2012-06-02 09:28 INOPIAE Relationship added related to 0001050
2013-05-01 11:27 INOPIAE Note Added: 0003951
2013-05-01 11:27 INOPIAE Assigned To => BenBE
2013-05-01 11:27 INOPIAE Status new => fix available
2013-05-01 12:50 BenBE Source_changeset_attached => cacert-devel testserver-stable f6744f8e
2013-05-01 12:50 INOPIAE Source_changeset_attached => cacert-devel testserver-stable 20163e4c
2013-05-01 12:50 INOPIAE Source_changeset_attached => cacert-devel testserver-stable a1b7678c
2013-05-01 12:51 BenBE Reviewed by => BenBE
2013-05-01 12:51 BenBE Status fix available => needs review & testing
2013-05-01 12:51 BenBE Product Version => 2013 Q2
2013-05-01 16:05 Werner Dworak Note Added: 0003966
2013-05-01 16:08 Werner Dworak Note Edited: 0003966
2013-05-06 13:04 JensK Note Added: 0003974
2013-05-06 13:05 JensK Note Edited: 0003974
2013-06-15 09:18 MartinGummi Note Added: 0004058
2013-06-15 09:20 MartinGummi Note Added: 0004059
2013-09-10 21:16 Eva Note Added: 0004304
2013-09-10 21:17 Eva Note Edited: 0004304
2013-09-10 22:40 NEOatNHNG Source_changeset_attached => cacert-devel testserver-stable bb149f72
2013-09-10 22:40 NEOatNHNG Source_changeset_attached => cacert-devel testserver-stable 502b87fd
2013-09-10 22:40 NEOatNHNG Source_changeset_attached => cacert-devel testserver-stable e3a3015c
2013-09-10 22:48 NEOatNHNG Reviewed by BenBE => NEOatNHNG
2013-09-10 22:48 NEOatNHNG Note Added: 0004307
2013-09-10 23:15 NEOatNHNG Source_changeset_attached => cacert-devel testserver-stable 5cca88e0
2013-09-10 23:15 NEOatNHNG Source_changeset_attached => cacert-devel testserver-stable f80065bc
2013-09-10 23:18 Eva Note Added: 0004309
2013-09-21 05:34 BenBE Note Added: 0004343
2013-09-21 06:00 BenBE Assigned To BenBE => egal
2013-09-24 22:45 Eva Note Edited: 0004309
2013-10-23 19:19 BenBE Note Added: 0004411
2013-10-23 19:30 BenBE Assigned To egal => INOPIAE
2013-10-24 13:45 Uli60 Note Added: 0004412
2013-10-24 14:11 Uli60 Note Added: 0004413
2013-10-24 14:15 Uli60 Note Edited: 0004413
2013-11-26 23:09 NEOatNHNG Reviewed by NEOatNHNG =>
2013-11-26 23:09 NEOatNHNG Status needs review & testing => needs work
2013-11-26 23:10 NEOatNHNG Assigned To INOPIAE => NEOatNHNG