[Freeipa-devel] Activation and password reset webapp UI

[Adam Young]

On 11/29/2011 08:57 PM, Ryan Thomson wrote:
> Hi Endi,
>
> Thanks for reviewing the patch. Looks like I have some work to do.
>
> 1-2) I have to admit I didn't even try building with these patches. I 
> was pretty sure install/Makefile.am would need modification to install 
> it but I didn't know if submitting patches to install/Makefile.am and 
> the spec file was frowned upon or not and I assumed wrong. I'll test 
> by building from git next time and making sure it's ready to go from 
> there on.
>
> 3) Strange. I suppose originally coding against IPA as packaged by 
> RHEL then "porting" to and testing against FreeIPA as packaged in F16 
> could why I didn't catch that. Gotta do everything against the latest 
> in git. Got it.
>
> 4) Box width will have to be increased in CSS to compensate unless we 
> want "Current Password" and "Verify Password" to get wrapped but no 
> big deal.
>
> 5-9) Noted.
>
> 10) Besides "finally" being redundant, error handling isn't great 
> overall, let me see if I can improve that.
>
> 11-13) Git newbie mistakes. I really don't know what I'm doing with 
> regards to git. I assumed sending multiple patches where it's just me 
> fixing earlier mistakes would be worse than jamming all the commits 
> into a single file. I kind of just wanted to send one patch to 
> implement everything but I have no idea how to do that besides get 
> everything "right" in a single commit. Is that how I'm supposed to do it?

I'd submit it all as one patch, as it isn't that big, and the work is 
fairly atomic.

Once you have it working as you want,  squash it.  We have a convention 
where we number the patches  using 4 digits.  So yours  would be 0001.  
And updated verson would be 0001-1  and the next fixed version would be 
0001-2.

Git is very forgiving.  Here's what I'd do.  Once you get it working 
right make two branches.  One will continue to have all of your changes 
in it in separate patches,  and you'll use the other one to squash them.


I'm going to assume that you did all of this in you home tree in your 
master branch.  So git checkout -b  activation_password  will create a 
branch with that name from the current point.  Then,  git checkout -b 
activation_password_squashed will do the same.  You can see your 
branches with git branch.  You can see which commit they refere to with 
git log <branchname>.   Then run git rebase -i HEAD~4  (assuming you 
have three commits.  Always one more than the number of commits)  and 
then use the vi commands to edit the commits into a single commit.

Once you have it all in a single commit, run the git patchformat command 
and submit that.

https://fedorahosted.org/freeipa/wiki/PatchFormat




>
> Might be a few weeks before I get another patch submitted to fix all 
> these amateur mistakes.


No problem.  Just make sure that your run git fetch ;  git rebase 
origin/master to bring your code in line with any changes made between 
now and then.

>
> Thanks,
>
> --Ryan
>
> On 11/29/2011 02:35 PM, Endi Sukma Dewata wrote:
>> On 11/28/2011 3:11 PM, Ryan Thomson wrote:
>>> Hello,
>>>
>>> Attached is my amateur attempt at contributing to FreeIPA.
>>>
>>> The patch implements an account "activation" and password reset webapp
>>> UI to cover use cases where FreeIPA may be acting only as a backend to
>>> services such as Samba or other web application that do no expose a
>>> method for changing expired kerberos credentials.
>>>
>>> This code is based on the "migration" webapp UI.
>>>
>>> See ticket 1907.
>>>
>>> https://fedorahosted.org/freeipa/ticket/1907
>>
>> Hi, thanks for the patch! I found some issues:
>>
>> 1. Build failed due to missing variables:
>>
>> install/activation/activation.py:94: [E0602, activate] Undefined
>> variable 'e'
>> install/activation/activation.py:97: [E0602, activate] Undefined
>> variable 'e'
>>
>> 2. After fixing #1, I found that the activation folder is not installed
>> into /usr/share/ipa. I think this needs to be fixed in
>> install/Makefile.am and freeipa.spec.in.
>>
>> 3. After copying the folder manually I can access the activation page,
>> but when test it I get this error in /var/log/httpd/errors_log:
>>
>> [Tue Nov 29 16:13:30 2011] [error] ERROR:root:migration context search
>> failed: {
>> 'info': 'TLS error -8172:Unknown code ___f 20', 'desc': "Can't contact
>> LDAP serv
>> er"}
>>
>> It probably should use ldapi instead of ldaps. See migration.py.
>>
>> 4. In index.html:33 I think the "Password" label should say "Current
>> Password" for better clarity. Also in line 41 the "Confirm" label should
>> say "Verify Password" for consistency with the rest of the UI.
>>
>> 5. In index.html:44 it should be </ul> instead of <ul>. This is an
>> existing problem in migration/index.html.
>>
>> 6. There are broken image links in ipa_activation.css. The images have
>> been slightly renamed and moved into install/ui/images recently.
>>
>> 7. There's a broken link to the CSS file in invalid.html:7.
>>
>> 8. The intall/activation/jquery-ui.css doesn't seem to be used anywhere.
>> This is also an existing problem in migration.
>>
>> 9. In activation.py:72 the error message still says 'migration'.
>>
>> 10. In activation.py:121 the return statement in the 'finally' clause is
>> redundant if there's an exception because it will return another page.
>> This return statement should be moved inside the 'try' clause. Also we
>> might need another return statement in the 'except' clause to handle the
>> case where the errno doesn't match EPERM or EIO.
>>
>> 11. The file actually contains 3 patches. It's better to put them in
>> separate files because in case one of them needs revision you won't have
>> to resend everything. Another option is to squash them into a single 
>> patch.
>>
>> 12. The second patch contains the first patch file.
>>
>> 13. There are some warnings about extra whitespaces when applying the
>> patch. Try applying the patch with --whitespace=fix then reexporting it
>> again.
>>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel