Re: task 8833 -- steps 3 and 4

[Davi Leal]

Federico Gimenez Nieto wrote:
> I have also extracted Donation.php from the patch to a separate file.

After some modifications it has worked!


Just some comments. The Donation.php was in DOS format!

DOS text files traditionally have carriage return and line feed pairs as their 
newline characters while Unix text files have the line feed as their newline 
character.

I used the 'dos2unix' command to convert it to Unix format.

Maybe it was due to you created such file. This is the first file you create. 
Other files where just modified.


> The additions:
>
>  * Modified addDonation method at Donation class: if the user is not
>    logged in, it creates the magic number and calls the getEntityId
>    with the $requestOperation and $magic parameters set. It also
>    inserts the donation with the proper D1_DonationMagic and
>    D1_DonationMagicExpires values. 
>
>  * Modified getEntityId method at Entity class: sends the verification
>    email if the email is registered and a donation addition has been
>    requested.

Proposed modifications to your patch:


 * Layer-0__Site_entry_point/doc/GNUHerds__SQL_Implementation.sql

     Replacing the below string:
       Donation pledge groups. -> Donations for donation-pledge-groups.

     Deleting whitespace at:
                 );


 * Layer-5__DB_operation/Donation.php

     Syntax error: Deleting extra ')' at:
                 // Logged in
                 $EntityId = trim($_SESSION['EntityId']);

     SQL query error at:
                 $sqlQuery = "PREPARE query(integer,text,integer,text)


 * Layer-5__DB_operation/Entity.php

     Move the below line to its scope, that is to say, where '$entity'
     is used:
                $entity = new Entity();

     Since this same call is made from more than one place, would
     it be better here a utility function?
        $magic = md5(rand().rand().rand().rand().rand().rand()...);
     It is just a line. IMHO adding a utility function would add
     complexity. I have remove such XXX-to-ask-for-opinion comment
     line.  Please, as usual, let the mailing list know if you
     disagree.

     Deleting trailing whitespace at:
                $message .= "\n\n";

     Deleting trailing whitespace at:
        }

     Proposed string modification:
       Donation verification -> Donation confirmation

     Proposal: Unify the strings to avoid more similar msgid to be
               translated.
               Generalize the expiration times too; All GNU Herds
               expiration times could be for example 48 hours.

          "Your email has been used to create or
           update a notice at GNU Herds"

        IMHO the subject is enough to specify what operation we are
        talking about. The email body can be something general and so
        we avoid adding more similar msgid to be translated. It is
        frustrating translate similar msgids.

          Examples of some GNU Herds email subjects:
            * Email verification
            * Donation confirmation



Instead of writing such proposed modifications by hand, you could just backup 
your current gnuherds-app directory and get a new git clone. Then do "git 
apply task-8833.4.diff". So we would be sure you do not miss some 
modification.

The Donation.php file is included in the attached patch too.



>  * 5: create entry point to verify donation, cloning the entity
>       verification process.

It seems creating a new entry point is not needed due to we could just process 
the 'action=', 'email=' and 'magic=' at [1], as we do for the entity's email 
validation at [2].

 [1] Layer-2__Business_logic/content/forms/FS_Donation_Pledge_Groups_form.php
       Process http://gnuherds.org/pledges

 [2] Layer-2__Business_logic/content/forms/Entity_form.php
       Process http://gnuherds.org/entity


URI
 action=donate
 address@hidden
 magic=63004535158eb97bc8157b6c21154321



As usual, more improvements could be added along the discussions.

What do you think about all this?

task-8833.4.diff
Description: Text Data