#1515 Add group button on dashboard
Merged by pingou. Opened by rahulbajaj.
rahulbajaj/pagure AddGroupButton  into  master

Download 1515.patch
no initial comment

rebased

rebased

Tests are failing apparently, did you run them?

rebased

@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 :)

rebased

@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)

While we are fixing this, could we also drop the modal in https://pagure.io/groups and just point to that new page?

@pingou i think that would be a great idea. keeping things consistent through out :).

rebased

rebased

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?

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?

rebased

As discussed on IRC with @pingou , I have tired to make some changes but it seems that jenkins is unhappy.

rebased

If there is no form in this page I do not think it should include a csrf token

@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)

rebased

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

Metadata