#556 Added watch functionality.
Closed by aavrug. Opened by aavrug.
aavrug/pagure new_ui  into  master

Download 556.patch
no initial comment

I'm still seeing a < :(

With the proper relation in the model we could drop this call, the method get_project_user and all the calls in the controllers to pagure.lib.check_watching(

I still think we should raise exceptions here where we return False.

Especially here, in which situation could the user not exist?

There is something odd, there seems to be a < here as well

I have two remarks for this section:

  • Where are the people watching the project being added to the list?
  • Using the proper relation in the model we could iterate through all the watchers/unwatchers and add/remove them as we go in a single method without the need for another explicit query to the DB.

Why the if on select here? I think it's defined on every page of the repo view, no?

data-user="{{ repo.user.user if repo.is_fork }}" looks simpler no?

You can make it a single line:

repo_obj = pagure.lib.get_project(SESSION, repo_name, repo_user if repo_user else None)

This should be a 404 error/page then

Ok so we still need the form here :)

So overall this looks good, I think we can simplify things quite a bit using a proper relation in the model and a few tweaks but it should be doable :)

It's got to be a bug in pagure, let's ignore these for now

I am trying to simplify these. Thank you for your review and the response.

Pingou please take a look, I added a new relation and did few tweaks.

Ok so for this it makes sense to have a separate query as otherwise it will slow things down.

This isn't complete:

  • Project you created aren't taken into account
  • Project you didn't create but have direct commit aren't taken into account
  • Project you have commit via a group aren't taken into account

We need another heuristic there

I kinda think we could find a better name but it's nitpicking.

Are these changes useful? pagure.css doesn't seem to be used in the new_ui branch anymore.

A proposal to remove all these duplicated lines:

in the Project class something like:

@property
def watchers_list(self):
    ''' Return the list of username of users watching this project. '''
    watchers = set([self.user.user])
    for user in self.users
       if user.default_email:
            watchers.add(user.default_email)
    for group in self.groups:
        if group.creator.default_email:
            watchers.add(group.creator.default_email)
        for user in group.users:
            if user.default_email:
                watchers.add(user.default_email)
    # Add here logic for project_users
    #  -> Add the people watching
    #  -> Remove the people who unwatched the project
    return list(watchers)

This way in the UI and where needed you can do something like:

if flask.g.fas_user.username in project.watchers_list

Are you saying that here I can use a query which will retrieve the object directly from the ProjectUser table filtered by project id and user id?

pingou I am ill and maybe I'll go on leave for few days so a little delay will happen from my side. Besides, I'll try to commit the codes as soon as possible.

No problem once the functionality will be completed I'll see how can I fix the CSS.

Take care and I hope you'll get better soon :)

Yes, it will avoid iterating over all the watchers to find one

(Imagine if we have a projects with 1000s watchers and we're looking for the last one, not a good idea to iterate here :) )

Sorry pingou for the delay. I pulled the repo and lots of things changed, should I continue with old setup or creating a new one will be a better idea?

Hey @aavrug, nice to see you back. I hope you're doing better :)

Regarding this PR it is up to you, depending if the pile of conflicts is manageable or not. Anyway, you can always force push to the same branch in your fork once you're done and the PR will be automatically updated.

  1. If a user is going to create a project or fork a project an entry will happen into project_users table in which watch column will be set true by default.
  2. If a user didn't fork or created a project, is this possible that he/she can commit?
  3. If a user is added to a group for a project same thing happen like 1.

Please suggest if it is a good idea or otherwise I'll look for a second option which will be a better idea than this.

If I create a pagure group, give it commit and add you to it, you'll have commit but nothing will be in project_users, it will go through project_groups

Pull-Request has been updated

Seems like there's still conflicts here.

@aavrug, @pingou, would you like to close this one for now, or keep it open?

@aavrug and I have been discussing this at length over the last few days, I expect aavrug will (force) push the new changes to this branch so that we can re-start the review process.

@aavrug, or we could close this PR and create a new one, this is up to you :)

Pull-Request has been closed by aavrug

Metadata