Feature/flag project #144

dtgay · 2015-04-16T15:43:40Z

This should fix #87, and moooreee! 🌈

Adds 'Flag' button to project pages
- Logged-in users can flag a project with a reason for doing so, such as "please delete this because blah blah blah"
Adds an admin view to review, sort, and open/close flags

pypingou · 2015-04-16T15:44:51Z

anitya/__init__.py

@@ -145,7 +145,8 @@ def log(session, project=None, distro=None, topic=None, message=None):
                             'project: %(project)s',
        'project.edit': '%(agent)s edited the project: %(project)s fields: '
                        '%(changes)s',
-
+        'project.flag': '%(agent)s flagged the project: %(project)s',


Should we include the message in there?

It's your choice. The reason might be kind of long, so I excluded it from the log message, figuring that an admin could check the reason from the Flags view itself. Do you think it'd be good to have it here, as well?

I think I would like to have it in yes. It'll give the admin an idea what this flag is about when looking at the list of flags.

The admin can already see the reason when looking at the list of flags. The reason is listed there. I just excluded it from this line to avoid a large block of text when viewing the logs. Do you think the full reason should be listed on the Flags and Logs page? If so, I can fix that.

Yes I think it would be useful.

As it is for admins only, we could still easily rework that later on :)

pypingou · 2015-04-16T15:49:55Z

anitya/lib/model.py

+        return output
+
+    @classmethod
+    def insert(cls, session, user, project, reason):


Looks like this is not being used anywhere

I just added a json representation in case it's used somewhere in fedmsgs or if it would be needed in the future. Should I remove it?

I meant the insert() method. It doesn't seem to be used anywhere.

The json representation will be useful for broadcasting fedmsg messages ;-)

dtgay · 2015-04-22T17:07:11Z

Alright, things seem to be working! Good to merge?

pypingou · 2015-04-22T17:17:03Z

To be entirely feature complete, it needs some unit-tests :-)

I also woul like to give it a try locally to see if I missed anything in the review :)

dtgay · 2015-05-11T20:24:28Z

@pypingou, I've added a good set of tests for both viewing the /flags/ view and attempting to set the flag state under various conditions. All of these tests appear to pass. Let me know what you think!

dtgay · 2015-05-11T20:42:23Z

Actually, there is one issue I'm seeing. In test_flag_project, we're getting 405's in these sort of blocks:

with anitya.app.APP.test_client() as c:                       
    with c.session_transaction() as sess:                     
        sess['openid'] = 'http://pingou.id.fedoraproject.org/'
        sess['fullname'] = 'Pierre-Yves C.'                   
        sess['nickname'] = 'pingou'                           
        sess['email'] = 'pingou@pingoured.fr'                 

    output = c.get('/flags/{0}/set/closed'.format(flag.id),   
                   follow_redirects=True)                     
    self.assertEqual(output.status_code, 200)                 

self.assertEqual(flag.state, 'closed')

I'm 99% sure this is because the CSRF protection makes it so you can't directly hit the URL. What do I have to change here to make this call in these tests?

pypingou · 2015-05-12T06:47:08Z

I see a couple of things here:

You're doing a GET request instead of a POST (cf c.get())
You will need to retrieve the csrf token first and include it in the data sent to the URL, see for example: https://github.com/fedora-infra/anitya/blob/master/tests/test_flask_admin.py#L96-L115

dtgay · 2015-05-12T18:32:25Z

I took a whack at getting the CSRF token, though I'm not sure I did it right. I thought I'd have to do it for each of those with anitya.app.APP.test_client() as c: blocks, but when I run nosetests, I don't get a failure. 😕

dtgay · 2015-05-19T17:19:22Z

I believe this is ready for final review.

pypingou · 2015-05-19T19:33:00Z

@oddshocks github seems to say this PR conflicts with what's in the repo, could you see if you could rebase it?

pypingou · 2015-05-20T13:59:11Z

anitya/lib/__init__.py

+
+    # Don't toggle the state or send a new fedmsg if the flag's
+    # state wouldn't actually be changed.
+    if flag.state == state: return


What about raising an exception here instead?

pypingou · 2015-05-20T14:11:42Z

Code wise, I have a couple of minor questions but this is looking really good, thanks @oddshocks !!

…eletion, etc

…e CSRF token from the previous block

dtgay · 2015-05-21T14:44:54Z

This should be good to finally merge, with final approval.

pypingou · 2015-05-21T14:48:39Z

I just tried to run the branch and I got:

  File ".../anitya/anitya/admin.py", line 447, in browse_flags
    total_page = int(ceil(cnt_flags / float(limit)))
UnboundLocalError: local variable 'cnt_flags' referenced before assignment

pypingou · 2015-05-21T14:49:49Z

Second one:

  File ".../anitya/anitya/ui.py", line 469, in flag_project
    plugins=plugins,
NameError: global name 'plugins' is not defined

:-)

pypingou · 2015-05-21T15:41:34Z

Nice job! 👍 for merging :)

Feature/flag project

pypingou reviewed Apr 16, 2015
View reviewed changes

dtgay force-pushed the feature/flag-project branch from dcf51a8 to c1a6b83 Compare April 16, 2015 15:49

pypingou reviewed Apr 16, 2015
View reviewed changes

dtgay force-pushed the feature/flag-project branch from d7c2d69 to 146028d Compare May 19, 2015 16:50

pypingou reviewed May 20, 2015
View reviewed changes

David Gay added 2 commits May 21, 2015 00:07

set up a form, template, and whatnot so users can flag projects for d…

d31fd7e

…eletion, etc

whoops, missing paren

4f3a677

David Gay and others added 16 commits May 21, 2015 00:08

actually don't need this

473c29b

Make a further csrf check in the tests here.

80caf85

need to get the CSRF token here, too, I believe

c00f72f

we want to reassign output for this block, so that we're not using th…

3c9027a

…e CSRF token from the previous block

this stuff needs to be tabbed back out of the session_transaction block

4bbe12b

raise an exception here, instead of just returning

73758c5

table header should be in a table row

5831b3e

clarify this text

374faad

remove unneeded switch

dd6d5fa

test a from_date of tomorrow

0cf83d4

don't need __init__ (handled by SQLA)

6f5dc24

I think this needs to be written like this for created_on to be defined

3cf558e

remove blank line

9822694

let's assertFalse here, instead

fb6e3be

there is no datetime.date.tomorrow()

6eff0be

check the from_date functionality in the browse_logs tests, too

0b50115

dtgay force-pushed the feature/flag-project branch from a16089e to 0b50115 Compare May 21, 2015 07:09

David Gay added 6 commits May 21, 2015 07:52

remove whitespace

bc6e0f9

remove more whitespace

882417e

this shouldn't be here for this view

26f4a2b

link back to the project page for each flag on the flags view

05fe69e

only show open flags by default

6b4ec0d

invert order of user and reason on flags page

fc6646a

dtgay pushed a commit that referenced this pull request May 21, 2015

Merge pull request #144 from fedora-infra/feature/flag-project

c29d327

Feature/flag project

dtgay merged commit c29d327 into master May 21, 2015

dtgay deleted the feature/flag-project branch May 21, 2015 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/flag project #144

Feature/flag project #144

dtgay commented Apr 16, 2015

pypingou Apr 16, 2015

dtgay Apr 16, 2015

pypingou Apr 16, 2015

dtgay Apr 16, 2015

pypingou Apr 16, 2015

pypingou Apr 16, 2015

dtgay Apr 16, 2015

pypingou Apr 16, 2015

dtgay commented Apr 22, 2015

pypingou commented Apr 22, 2015

dtgay commented May 11, 2015

dtgay commented May 11, 2015

pypingou commented May 12, 2015

dtgay commented May 12, 2015

dtgay commented May 19, 2015

pypingou commented May 19, 2015

pypingou May 20, 2015

pypingou commented May 20, 2015

dtgay commented May 21, 2015

pypingou commented May 21, 2015

pypingou commented May 21, 2015

pypingou commented May 21, 2015

Feature/flag project #144

Feature/flag project #144

Conversation

dtgay commented Apr 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtgay commented Apr 22, 2015

pypingou commented Apr 22, 2015

dtgay commented May 11, 2015

dtgay commented May 11, 2015

pypingou commented May 12, 2015

dtgay commented May 12, 2015

dtgay commented May 19, 2015

pypingou commented May 19, 2015

Choose a reason for hiding this comment

pypingou commented May 20, 2015

dtgay commented May 21, 2015

pypingou commented May 21, 2015

pypingou commented May 21, 2015

pypingou commented May 21, 2015