http://img.susepaste.org/46916846

http://img.susepaste.org/34190879

http://img.susepaste.org/38620663

http://img.susepaste.org/31082825

I think it would be a no brainer to read the code if you add else here.

This function is used to star or unstar a project. Maybe it should be documented in the docstring since the name of the function is not making it clear.

I am not sure the title is needed, I think it is obvious.

You could just use Star this project as title

Here the title could just be Unstar this project

Same as above :)

Added a few minor comments.

I get your point but, i don't really like the unnecessary else statement :/ I even remove them if the if part is returning :(

It's normally considered cleaner to have as little return statement as possible.
So in this case I would agree with @cverna and advice that we use an if/else construct and have a single return msg at the end.

liked or starred?

Should we check that stargazer_obj exists before we delete it?

Do we have a way to name that constraint as you did in the alembic migration?

We'll need tests for all this :)

:+1: this is a feature that was required :smile:

@pingou , @farhaan , i have added tests but, my unit tests are not fully fixed, there are some errors which i doubt is related to star project. Please check before merging :p

i should tag @cverna as well. :D

missing the star arg from the docstring.

Why not have star as a boolean here ?

repo to unstar

missing the return docstring

missing the return docstring

missing the return docstring

need to update the repo and user docstring

missing the return docstring

maybe use the same convention as above and put the type in the arg description

I think it would be nicer to have star as a boolean.

same here star would be a good boolean :)

Maybe you put the type in the arg description. And elsewhere we use arg instead of param

I would change this for a boolean

identation feels odd here

if star is a boolean you can drop this

start ? :)

I would maybe create a separate class for the ui testing so that you can take advantage of the setUp method to star the project

2017 sir, you are out of college now :/

:thumbsup: :D

Needs a rebase too :)

@vivekanand1101 http://paste.opensuse.org/66275115 :(

This is on my laptop 1080p resolution on a 13.3" screen

this is how it looks in mine. http://paste.opensuse.org/20249430 . If you want to fix (pls :cry: ) you already have access to my fork.

http://img.susepaste.org/28725340

:thumbsup:

I was just trying to test this, did both of you tried the alembic migration on postgresql? I'm running into a weird NotImplementedError error

downgrading and upgrading is working fine for me :/ https://paste.fedoraproject.org/paste/wqU27dPDBasCVUoR5zgz~Q

I had to remove sa.MetaData(), to get it working here :(

called alembic with PAGURE_CONFIG... or did you adjust the default_config?

I ruined it :/

aouch, quite a few tests are failing here :(
Do they work for you?

@vivekanand1101 do you need help with fixing the tests ?

@cverna yes sir

@vivekanand1101 do you need help with fixing the tests ?

I got most of them passing, seeing just one in the current run