Missing indentation for the content of the form :)

Instead of having these in the form w/ hidden fields, maybe we could place them in the URL as we do in other places

repo_user can be None, that's no problem, we just need to make sure it isn't '' (ie: empty string)

Could/Should we check the value of watch?

Well this doesn't tell us much since the URL doesn't exists whether a foo project exists or not :)

It redirects you, but you're not checking the output, you should follow_redirects=True and see what's in the HTML. Did the Watch flag changed? Was there a message flashed? Did it redirect you to the right page?

Check the ConfirmationForm ;-)

Check the login method, there is a way to check that the previous_url is sane

repo cannot be None, otherwise we would have a 404 since the URL wouldn't hit here :)

Let's make this str(watch) to be sure :)

No need for the if user, just specify it, if user is None, it'll know what to do, check the code ;-)

rollback() shouldn't be needed for a PagureException but it would for a sqlalchemy error

Split this string over two lines? (same below?)

Whenever you get time just review this. I have taken so much time for this issue, so now thinking that if this will merge then I will move to the next issue.

This should be taken care of in notify.py no?

No that is for the user is watching or not for watch/Unwatch button.

Let's be consistent about our URL schemas:

I propose:

@APP.route('/<repo>/settings/watch', methods=['POST'])
@APP.route('/fork/<username>/<repo>/settings/watch', methods=['POST'])

Yes but we remove all the users not watching in notify.py if I read the code correctly

https://pagure.io/pagure/pull-request/843#_5,15 that is for this and It is not related to notify.

Ah I see, but then we are iterating over a potentially long list of watchers while in fact we are only interested in one person, the person who is logged in. Wouldn't it make more sense to check just this person instead of iterating through all of them?

Yea that's why previously I created a method check_user in the deleted PR and you told me that it is not a good way and have to remove so, I removed it. Could you please help me out with this? I can create a method that's not an issue please suggest me what is a good way.

Arf, sorry I probably mis-understood your idea then :(

I think we could make an is_watching(session, username, project)
method in pagure.lib and let it return a boolean if the specified user
is watching the given project.

No problem :)
If this will be the way then I have to set variables in the methods from which I can set the watch and then I can use it in repo_master.html.

Looking at the code in this PR, if I create a repo, if I'm added to it or if I'm added to a group who has commit, no entries is made to the watchers table, so we need a little more logic here to determine if the user logged in is considered to be watching or not.

Hello Pingou,

I was little busy with my office work so didn't ping you, sorry. If anything left on this task please let me know or I'll move to next one.

We can skip this loop if watch is already True

yes, absolutely.

Is this still used? I'm not finding it right now (but I might be missing it)

That's not my code there must be something wrong. Most probably I did something wrong when I was fixing the conflicts. What I have to do now Pingou. Sorry for the mistake.

If 'watch' is a boolean, there is no need to convert it to an int

Indentation error here

I still try to get the lines not too long, we should try to reduce this one

This is already present below, so it breaks flask, we need only one

The PR pages and Issues pages don't have the watch button set correctly

This fails: AttributeError: 'set' object has no attribute 'delete' which means we're missing unit-tests for this

When I committed this was not there. I'll remove that.

I am not getting your point.

Ok I'll check it.

We had a discussion on this and you suggested this would be a good idea to parse.

If it's already a boolean, it's not needed, the question is: is it a boolean :)

The point is that the line is too long :)

Better message syntax: 'You are now watching this repo.'

Same as above, better message syntax: 'You are now'

'You are now'

ok thanks

No it's not boolean.

this fails if not logged in (AttributeError: 'NoneType' object has no attribute 'username'). I guess fas_user is null when you are not logged in. But we should still be able to view a project.

I assume will be the same as above

same as above

same as above

same as above

same as above

same as above

same as above

same as above

same as above

I believe @pfrields suggested: Better message syntax: 'You are now watching this repo.'

It has been pointed out that these lines will not work when not logged in

Maybe we should require the user to be logged in, otherwise line 140 below will not work

My bad, messages just disappear that's why I forgot. I will change that.

It was fine I tested, previously it was flask.g.fas_user.username that's why giving error. Now it is working just fine.

I didn't faced any issue but still I'll test it.

Ok, I see the change you made to fix this :)

If the user is not logged in, the watch option will not appear.

true but the endpoint remains accessible, for example via python requests :)

indentation is off here

If you fix that last comment and rebase I'll merge :)