Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RDISCROWD-5412 Search contacts against coowners, subadmin, admin. #776

Merged
merged 10 commits into from
Sep 19, 2022

Conversation

kbecker42
Copy link

@kbecker42 kbecker42 commented Sep 13, 2022

  • Added parameter to allow searching for contacts against coowners, admin, subadmin.

Screenshots

contacts-2

Re: bloomberg/pybossa-default-theme#376

Copy link

@XiChenn XiChenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the unit test failed due to the change.

Copy link

@XiChenn XiChenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you fix the unit test?


if form.contact and form.contact.data.lower() == 'true':
# Filter contacts only to users that are enabled and assigned to this project or are sub-admin/admin.
users = [user for user in users if is_user_enabled_assigned_project(user, project) or user.id in project.owners_ids]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, only users that are part of project.owners_ids to be contacted and admins/subadmins not part of project.owners_ids need not be considered.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, user in contact list when removed from owners_ids should also be removed from contacts list

Copy link
Author

@kbecker42 kbecker42 Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep this PR simple, perhaps we should allow the user to manage the lists independently. That is, after removing a co-owner, they would also need to remove the contact as well (two clicks), followed by saving.

Otherwise, if we choose to keep the contacts and co-owners synchronized, it may require modifying the Vue component to react to the 'X' button on both lists in the UI, in addition to including the filter in the server-side code. Perhaps, this could be postponed to a future ticket?

This would help keep the scope of the work involved in this PR contained. Thoughts?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, having future ticket to handle follow up work to synchronize contacts with co-owners is fine. thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coveralls
Copy link

coveralls commented Sep 15, 2022

Pull Request Test Coverage Report for Build 3083355893

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 94.82%

Totals Coverage Status
Change from base Build 3019262673: 0.003%
Covered Lines: 16273
Relevant Lines: 17162

💛 - Coveralls

pybossa/view/projects.py Show resolved Hide resolved
for user in users:
if user.id in project.owners_ids:
filtered_list.append(user)
users = filtered_list
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you prefer, line 3201 to 3205 can be rewrite with list comprehension.

users = [user for user in users if user.id in project.owners_ids]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed via c8440d7

pybossa/view/projects.py Show resolved Hide resolved
Copy link

@XiChenn XiChenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants