rebased
Tests are failing apparently, did you run them?
@pingou thanks for pointing it out :)
Looking at it it seems good.
Could you rebase? I'll try to have a look at it locally before merging :)
@pingou rebased :)
This looks quite good.
While we are fixing this, could we also drop the modal in https://pagure.io/groups and just point to that new page?
@ryanlerch what do you think of this? ^ (dropping the modal)
@pingou i think that would be a great idea. keeping things consistent through out :).
Looks like jenkins is unhappy (and locally tests are failing as well)
@pingou can you help me with this, i am not sure as to what changes are to be made in the test_pagure_flask_ui_groups.py
The first question is: can you reproduce the issue?
I dont know, when i run the tests it says 'indexError: out of bound index'. On the other hand the modal seems to be disappeared and the link to the 'new page' is correctly directed when I click the 'New Group' button on https://pagure.io/groups.
One thing i noticed is, Although when i add the link to direct to new page, if i let the code for modal be there as it is and not delete the code - the tests run fine. But i feel when we are not using the modal then keeping its code makes no sense.
Did you print the content return by that piece of code in the tests? Do you follow what's going on?
Clearly the code for the modal should be removed, but this has consequences on the tests. Do you see why?
As discussed on IRC with @pingou , I have tired to make some changes but it seems that jenkins is unhappy.
If there is no form in this page I do not think it should include a csrf token
ping?
@pingou hey, knowing that: a) the 'group_list.html' is not having a form to add a csrf token, b) as the csrf token is removed which was present in the modal, gives an error in the local test.
I am trying to add a csrf token somewhere to make the tests the run and also by not adding the csrf token in the 'group_list.html'.
@rahulbajaj you're forgetting one option: get the csrf token somewhere else for the tests :)
@pingou hey, i am trying to understand. if i am not wrong, we use csrf_token only for forms right? like when we take input from the user. All the forms already have a csrf token with them, i dont understand where can i add one.
or, i since the tests are looking for a csrf_token which is deleted, i can make changes in the tests to point that csrf_token to some other part of the code. Is this what is expected ?
we use csrf_token only for forms
Correct
All the forms already have a csrf token with them, i dont understand where can i add one
But there is no longer a form on that page
i can make changes in the tests to point that csrf_token to some other part of the code
That's a suggestion, since there is no CSRF token on the page (which is correct since there is no longer a form on that page) and since we need a CSRF token, we need to go get one somewhere else (in the tests)
Looking great! I'll want to test it quickly locally but code-wise it looks good :)
Tested locally, works flawlessly :)
Pull-Request has been merged by pingou