pagure

#1510 Add the possibility to subscribe/unsubscribe to specific issues
Merged by pingou. Opened by pingou.
subscribe  into  master

Download 1510.patch

Fixes https://pagure.io/pagure/issue/1293

4 new commits added

  • Fix running the test
  • Let's find the acl.id automatically instead of hard-coding it
  • Add unit-tests for api_subscribe_issue
  • Add the issue_subscribe ACL to the list of ACLs

Did you mean "Objet" or "Object"? "Objet" is used consistently in the code, but it looks a bit odd.

Object indeed :)

1 new commit added

  • Rename InvalidObjetException to InvalidObjectException as found by @tibbs

1 new commit added

  • Add debug code for jenkins

1 new commit added

  • Specify in which direction we want to order the dates

1 new commit added

  • Some more debugging for jenkins

1 new commit added

  • Again more debugging for jenkins

The docblock indicates that the username should be passed in the POST body, but it looks like the authenticated username is used instead. I actually like this behavior more because users probably don't want other users to be able to change their subscriptions. Maybe remove username from the docblock?

Are all three of the above print statements stray?

true there, for both endpoints even I think

for now, they are debugging statements for jenkins (see the last commit messages :))

This seems like a stray statement as well.

Is there a reason to include the username since the view uses the username from the session?

yes it's needed for retrieving the right project

yup

It would be cleaner to break each test condition into its own test, so each test is shorter/easier to understand and separately concerned.

Stray print.

It also means re-creating the environment for the tests each time which is going to end up time-consuming (tests are already taking ~15minutes on jenkins these days!)

And the name of the function are going to end up over the 80 chars line, no? :D

It would also be cleaner to break this up into several tests for the various conditions.

You can use the class level set up method instead of setUp() to do the setup once to save time, if you want. That would save some time but still allow your tests to be more atomic/readable.

I personally like 100 columns because I don't want 1980's monitors to restrict my ability to be expressive with my variables/methods, but that's just an opinion ☺ It's the one part of PEP-8 that I explicitly choose to violate (I like 100 columns instead).

The only thing I think needs to be changed before merging is to remove the documentation that says that username is used from the POST body since it is used from the session (which I think is the ideal behavior anyway). Everything else I said is just a suggestion that you can feel free to ignore if you like.

LGTM.

1 new commit added

  • Try a different ordering

1 new commit added

  • Try a different ordering, ascending this time

1 new commit added

  • Fix typo in the exception raised

30 new commits added

  • Remove remaining debugging code
  • Expand the doc on api_view_user_activity_date
  • Drop the username from the documentation of api_subscribe_issue
  • Fix typo in the exception raised
  • Order the activity entries by their id rather than relying on dates
  • Rename InvalidObjetException to InvalidObjectException as found by @tibbs
  • Fix running the test
  • Let's find the acl.id automatically instead of hard-coding it
  • Add unit-tests for api_subscribe_issue
  • Add the issue_subscribe ACL to the list of ACLs
  • Attempt to fix running the tests on jenkins
  • Add first unit-tests for pagure.lib.notify
  • Fix running unit-tests for the change in the methods
  • Make set_watch_obj and is_watching_obj raise an exception for odd objects
  • Add/Remove people who explicitly asked to be/not be notified
  • Rename _get_emails_for_issue to _get_emails_for_obj
  • Correctly present the subscribe and unsubscribe buttons in the UI
  • Let is_watch_obj accept username or email as well as an user object
  • Add the subscribe/unsubscribe button on the issue page
  • Add a new API endpoint to toggle subscription on an issue
  • Fix the logic in is_watching_obj
  • Fix properly setting the watch status on an issue or a PR
  • Fix the foreign key on the watchers tables
  • More tests around pagure.lib.set_watch_obj
  • Let's raise a dedicated exception if we end up with an odd object to watch
  • Rename pagure.lib.watching_obj to pagure.lib.set_watch_obj
  • Add unit-tests for pagure.lib.watching_obj
  • Add unit-tests for pagure.lib.is_watching_obj
  • Add method watching_obj and is_watching_obj to pagure.lib
  • Add the issue_watchers and pull_request_watchers tables in the model

Ok, I think I got all of your remarks and cleaned up the debugging commits.

I'll wait for jenkins to say something and if it's clear I'll merge.

Thanks for your review!

1 new commit added

  • Small pep8 fix

Jenkins is happy, let's merge!
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.