Feature add tc #467

ChaamC · 2021-09-21T15:06:59Z

Features / Changes

Add new Terms and conditions field for group creation. When a request is made to assign a user to a
group with terms and conditions, an email is now sent to the user with the terms and conditions. The user
is assigned to the group when receiving the user's approval of terms and conditions, and another email is
then sent to notify the user of the successful operation.
Changed /groups/{group_name}/users, /users/current/groups and /users/{user_name}/groups endpoints with
a new parameter to either get active, pending or all users or groups. This new parameter is useful to
display any pending users/groups on the UI.

Bug Fixes

Fix HTTP Internal Server Error [500] on the page to edit a group when deleting the last user of a group.

Other references

Implements feature DAC-119

…ature-add-TC

…n case of confirmation

…ptation

… a logged non-admin user

…d keeping irrelevant tokens

…users/groups

… pending users, to allow sending additional email requests

…_group page

…ages + fix test for 'pending' display on ui

…ature-add-TC

codecov-commenter · 2021-09-21T16:18:15Z

Codecov Report

Merging #467 (decc9e2) into master (0c3ea91) will increase coverage by 0.03%.
The diff coverage is 73.13%.

@@            Coverage Diff             @@
##           master     #467      +/-   ##
==========================================
+ Coverage   79.45%   79.49%   +0.03%     
==========================================
  Files          70       70              
  Lines        8909     9061     +152     
  Branches     1211     1226      +15     
==========================================
+ Hits         7079     7203     +124     
- Misses       1544     1573      +29     
+ Partials      286      285       -1

Impacted Files	Coverage Δ
magpie/api/notifications.py	`65.27% <ø> (ø)`
magpie/register.py	`46.47% <ø> (ø)`
magpie/ui/management/views.py	`46.55% <13.33%> (-1.84%)`	⬇️
magpie/ui/user/views.py	`63.96% <47.36%> (-3.75%)`	⬇️
magpie/services.py	`81.20% <73.68%> (-0.14%)`	⬇️
magpie/__meta__.py	`100.00% <100.00%> (ø)`
magpie/adapter/__init__.py	`62.79% <100.00%> (ø)`
magpie/adapter/magpieowssecurity.py	`69.72% <100.00%> (+0.56%)`	⬆️
magpie/adapter/magpieservice.py	`63.63% <100.00%> (ø)`
magpie/api/management/group/group_formats.py	`100.00% <100.00%> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c07b5b...decc9e2. Read the comment docs.

fmigneault

Very nice PR and great tests!
I would like only a modification regarding the API endpoints (see comment in CHANGES).
Need a missing section in docs.
Code of the feature look all good.

fmigneault · 2021-09-21T15:50:39Z

CHANGES.rst

+  ``group`` with terms and conditions, an email is now sent to the ``user`` with the terms and conditions. The ``user``
+  is assigned to the ``group`` when receiving the ``user``'s approval of terms and conditions, and another email is
+  then sent to notify the ``user`` of the successful operation.
+* Add new requests under ``/groups/{group_name}/pending_users``, ``/users/current/pending_groups`` and


Instead of adding a new paths, I would prefer that existing ones receive a query parameter similar to the one for pending users.
This would allow us to either query "active" users/groups, pending ones, or both.
A field pending: true|false could to be added to returned user objects to indicate the pending state when querying "all" ("active"+pending).
The default when the query is omitted should be to return only active users/groups.

So if I understand, for example, I can move the code of /groups/{group_name}/pending_users to /groups/{group_name}/users, and add a new input parameter to the /groups/{group_name}/users query, indicating what type of users to get.
I could make an ExtendedEnum with different usergroup types :
ALL_USERGROUPS (which adds to each returned user the boolean pending field you mentioned)
ACTIVE_USERGROUPS
PENDING_USERGROUPS

I can add a user_type parameter to the input of the GET query, which contains an user_type enum value corresponding to one of the entry above.

Yes. Exactly what I add in mind. You should only need to remap the detected "pending" to your function that was defined in current pending_users endpoint, and if "all" was requested, extend the result with the usual operation.

docs/glossary.rst

magpie/api/schemas.py

magpie/api/management/user/user_utils.py

magpie/api/templates/email_group_terms_submission.mako

fmigneault · 2021-09-21T17:31:58Z

magpie/ui/management/templates/add_group.mako

+            <td>Terms and conditions:</td>
+            <td>
+                <label>
+                    <input type="text" name="terms" value="${form_terms}" />


Hard to know with static analysis.
Is this field sufficiently large for convivial writing of long blocks of T&C text?
I think the field is by default not expandable and single line.

Might need to increase its size for this case + ensure text wrapping is used to help writing paragraphs instead of horizontally scrolling text box.
You can make a specific CSS class to apply needed styles to apply this behaviour if other similar cases arise.

Good idea, I didn't think to test for long blocks of text, I will look into it!

I am currently checking to make it better for multiline text. I use a textarea, that can be resized only vertically. By default, it displays 4 rows, but can be resized to display more, and can be scrolled through too. What do you think of the layout?

This is just a fast draft, but I think I should make the Terms and conditions and its associated (optional) label aligned with the top of the textarea.

Also, I should align the (optional) label of the description beside its input text box, and not aligned with the textarea of the T&C.

There is the Add Group button that should stay aligned as before, with the input text boxes of Group name and Description, or it will move depending of the horizontal size of the textarea. You can see it has a weird alignment on the current screenshot.

Also, what do you think of the horizontal size of the textarea? I didn't want to make the same size as the others, as it should contain more text and should be larger. I think I should leave the horizontal resizing enabled too, so the user can use the width he wants.

Let me know if my idea makes sense or if you have any better suggestion so I can work on a proper version of the ui!

I've added a minimal width and height, to avoid weird formatting, and realigned the different elements.

Here is how it looks, by default, when opening the page :

Here is an example with the minimal size (the default width is a bit larger) :

Here is an example with the expanded textarea (the optional labels and the Add Group are still aligned in the right way):

I use a textarea, that can be resized only vertically. By default, it displays 4 rows, but can be resized to display more, and can be scrolled through too. What do you think of the layout?

Excellent!

This is just a fast draft, but I think I should make the Terms and conditions and its associated (optional) label aligned with the top of the textarea.

I totally agree.

Also, I should align the (optional) label of the description beside its input text box, and not aligned with the textarea of the T&C.
Also, what do you think of the horizontal size of the textarea? I didn't want to make the same size as the others, as it should contain more text and should be larger. I think I should leave the horizontal resizing enabled too, so the user can use the width he wants.

The last screenshot looks perfect.
If (optional) works by automatically following resizes of T&C, let's keep that design.

magpie/ui/management/templates/edit_user.mako

magpie/ui/management/templates/edit_group.mako

magpie/ui/user/templates/edit_current_user.mako

…ing T&C, but not as pending

ChaamC · 2021-10-19T14:52:11Z

@dbyrns @fmigneault
PR updated according to latest feedback. Let me know if it is okay, or if the latest changes require further adjustments!

fmigneault

Only minor tweaks left. The overall features is valid.

(optional) should stick to the left (closer to its text field) in this case.

fmigneault · 2021-10-20T18:06:23Z

magpie/api/management/user/user_views.py

+    user_info = uf.format_user(user)
+
+    # indicate if user has any pending T&C groups
+    group_names = uu.get_user_groups(user, UserGroupStatus.PENDING, request.db)
+    user_info["has_pending_group"] = bool(group_names)
+


Instead of adding has_pending_group here, move it within format_user to always make it available.
It should be within this block

Magpie/magpie/api/management/user/user_formats.py

Lines 42 to 44 in a54c89f

if not basic_info:

grp_names = group_names if group_names else [grp.group_name for grp in user.groups]

user_info["group_names"] = list(sorted(grp_names))

Alright, it's good idea, I will move it!

A thing that bugs me here, is that the code to get the pending_groups uses a db_session :
group_names = uu.get_user_groups(user, UserGroupStatus.PENDING, request.db).
But we don't have a db_session in the format_user method. Is there a way to obtain a db_session just for the format_user method, or should I maybe find a way to get pending users without using a session?

Mmhm, also getting a circular import error with from magpie.api.management.user.user_utils import get_user_groups :

magpie/api/webhooks.py:16: in <module> from magpie.api.management.user.user_formats import format_user magpie/api/management/user/user_formats.py:7: in <module> from magpie.api.management.user.user_utils import get_user_groups magpie/api/management/user/user_utils.py:30: in <module> from magpie.api.webhooks import (

This is why I proposed last time that this operation should be a method within User.
The User object will have an handle to the current db session and the method will be accessible from within format_user. It should behave similarly to the other available method user.groups.

To be clear, when you mention User, do you mean in this class?

Magpie/magpie/models.py

Lines 173 to 175 in a54c89f

class User(UserMixin, Base):

def __str__(self):

return "<User: name={} id={}>".format(self.user_name, self.id)

So, I would have to add a method has_pending_group with @declared_attr decorator, which could be accessed as user.has_pending_group in the format_user function?
Let me know if I understand properly.

But I checked briefly and am not sure where the session is accessible from there. I see that other attributes that use a session require a session as input parameter. Or do I have to use the get_db_session() method?

Also, including from magpie.api.management.user.user_utils import get_user_groups in that file also creates the circular import error.

tests/test_magpie_api.py:15: in <module> import tests.interfaces as ti tests/interfaces.py:19: in <module> from magpie.api import schemas as s magpie/api/schemas.py:29: in <module> from magpie.models import UserGroupStatus, UserStatuses magpie/models.py:32: in <module> from magpie.api.management.user.user_utils import get_user_groups magpie/api/management/user/__init__.py:2: in <module> from magpie.models import UserFactory

Anyway, I will look into it tomorrow, but let me know if I understood properly where you would see the location of the has_pending_group code, to be sure I am on the right track. We can figure out the rest after, or call each other tomorrow if needed.

So, I would have to add a method has_pending_group with @declared_attr decorator, which could be accessed as user.has_pending_group in the format_user function?

Exactly.

For the session, you can use get_db_session(obj=self).

The get_user_groups could also be a method added to Users.
Something like groups_by_status to avoid conflict with groups attribute.
You could probably even improve user.has_pending_group by simply having it call this user.groups_by_status method with bool() on the result.

Thanks for the feedback, it helps! I will try and implement it this way.

magpie/api/schemas.py

ChaamC · 2021-10-20T19:35:11Z

@fmigneault

(optional) should stick to the left (closer to its text field) in this case.

Mmhm, I just tested the ui to be sure, and I don't get the same result. It seems to be working fine on my setup, using Google Chrome Version 90.0.4430.212 (Build officiel) (64 bits) and Firefox 78.8.0esr (64 bits). There are other things different in your screenshot : Terms and conditions label and related (optional) label are not aligned with the top of the T&C box, and the Add Group button is not aligned with the left of the T&C box.

Here is what I currently have on my setup :

Actually, your screenshot looks more similar to the ui result I would get before I added the html/css adjustments.
See the screenshot I had done a few weeks ago on the PR here :

Are sure you had the latest changes, or what setup did you use to test the ui? Is there something that could cause the ui to fail only on certain specific setups?

fmigneault · 2021-10-20T20:38:44Z

@ChaamC
I was on commit a54c89f but maybe some css caching was happening.
I retried with dev-toolbox/no-cache and it looks fine now.

magpie/alembic/versions/2021-06-09_cb92ff1f81bb_add_group_terms.py

…rors when handling users

fmigneault · 2021-10-26T02:34:11Z

magpie/models.py

@@ -174,6 +174,43 @@ class User(UserMixin, Base):
    def __str__(self):
        return "<User: name={} id={}>".format(self.user_name, self.id)

+    def get_user_groups_by_status(self, status, db_session=None):


Rename to only get_groups_by_status. Don't need to repeat "user" since it is contextual to it.

fmigneault · 2021-10-26T02:42:36Z

magpie/api/management/user/user_utils.py

@@ -205,7 +205,7 @@ def update_user(user, request, new_user_name=None, new_password=None, new_email=
        if update_email_admin_only and not (update_username or update_status):
            err_msg = "User email update not permitted by non-administrators when email registration is enabled."
        ax.verify_param(get_constant("MAGPIE_ADMIN_GROUP", request), is_in=True,
-                        param_compare=get_user_groups_checked(request.user, request.db), with_param=False,


Revert this change.
All "<>_checked" operations imply more validations than just fetching relations between models.
They also imply raising HTTP errors which are irrelevant if raised in another context that within this kind of call. These checks should not be placed within the User model.

Done, I put the get_user_groups_checked function back to user_utils.py.
I had to import it directly into the get_groups_by_status function in models.py in order to avoid another circular import error.

magpie/ui/home/static/style.css

fmigneault

@ChaamC
All good.
Thanks for your work.

cwcummings added 20 commits June 10, 2021 15:03

add terms column to groups table + test if terms is immutable

b1dee6a

add terms field to group creation page

8a248dd

add terms and conditions info (uneditable) to the Edit Group page

7bb36de

Merge branch 'master' of https://github.com/Ouranosinc/Magpie into fe…

8d0781e

…ature-add-TC

check for terms on adding usr to group

e637e55

send email for T&C acceptation for group with terms + use tmp_token i…

ea14913

…n case of confirmation

send email to confirm user has been added to group following T&C acce…

37b77c3

…ptation

reorder tests and add t&c support for joining a discoverable group as…

e2cda93

… a logged non-admin user

add api endpoints to get pending group memberships

9ee4b0a

adapt ui to display pending users

82fcb00

bugfix: remove all identical tmp_tokens upon T&C confirmation to avoi…

ebaca98

…d keeping irrelevant tokens

remove any user/group that is already confirmed when getting pending …

4bf05de

…users/groups

add comment for checkbox that remain enabled to join groups, even for…

553aa90

… pending users, to allow sending additional email requests

bugfix: 500 error when removing last user from a group, from the edit…

8ef6dd2

…_group page

bugfix pending not displayed until refresh on edit_user and account p…

51ae1f0

…ages + fix test for 'pending' display on ui

review api documentation

984b1a0

document group definition with terms and conditions feature

1b8e698

update changelog

d986b4a

fix lint errors

acdc4fb

Merge branch 'master' of https://github.com/Ouranosinc/Magpie into fe…

5b6f7b7

…ature-add-TC

cwcummings added 2 commits September 21, 2021 11:50

review code

2c8c2e9

add ref to send_email call in user_utils, for mocking new tests

c9bede9

change string formatting for py3.5 support

f247cb5

fmigneault requested changes Sep 21, 2021

View reviewed changes

ui T&C test : check if group/user is still found in body after accept…

a54c89f

…ing T&C, but not as pending

ChaamC requested a review from fmigneault October 19, 2021 14:52

dbyrns previously approved these changes Oct 19, 2021

View reviewed changes

fmigneault assigned ChaamC Oct 20, 2021

fmigneault reviewed Oct 20, 2021

View reviewed changes

update create group post request descriptions

6d30dcb

ChaamC dismissed dbyrns’s stale review via 6d30dcb October 21, 2021 13:16

fmigneault requested changes Oct 21, 2021

View reviewed changes

magpie/alembic/versions/2021-06-09_cb92ff1f81bb_add_group_terms.py Outdated Show resolved Hide resolved

cwcummings added 5 commits October 22, 2021 09:46

update alembic revision id

fd8faa3

reorganize has_pending_group user info

14c5c23

fix lint errors

eb884e2

add empty get_user_groups_by_status method to UserPending to avoid er…

3e55c9e

…rors when handling users

fix new lint errors with stylelint 14.0.0 update

5752faf

github-actions bot added the ci Something related to code tests, deployment and packaging label Oct 25, 2021

fmigneault mentioned this pull request Oct 25, 2021

add support of Twitcher 0.6.x for MagpieAdapter #475

Merged

small correction in docstring

7bc3e24

ChaamC requested a review from fmigneault October 25, 2021 18:59

fmigneault requested changes Oct 26, 2021

View reviewed changes

cwcummings added 4 commits October 26, 2021 09:10

rename get_user_groups_by_status function

8eb225e

reset changes on get_user_groups_checked function

287ec83

fix lint error

b1ea5d4

remove css 3-values notations, using 4-values instead

decc9e2

ChaamC requested a review from fmigneault October 27, 2021 12:56

fmigneault approved these changes Oct 27, 2021

View reviewed changes

ChaamC merged commit e747a62 into master Oct 27, 2021

ChaamC deleted the feature-add-TC branch October 27, 2021 19:54

fmigneault mentioned this pull request Oct 27, 2021

💡 [Feature] SMTP configuration for services that need it bird-house/birdhouse-deploy#202

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature add tc #467

Feature add tc #467

ChaamC commented Sep 21, 2021 •

edited

Loading

codecov-commenter commented Sep 21, 2021 •

edited

Loading

fmigneault left a comment

fmigneault Sep 21, 2021

ChaamC Sep 22, 2021 •

edited

Loading

fmigneault Sep 22, 2021

fmigneault Sep 21, 2021

ChaamC Sep 22, 2021

ChaamC Sep 22, 2021

ChaamC Sep 22, 2021 •

edited

Loading

fmigneault Sep 22, 2021

ChaamC commented Oct 19, 2021

fmigneault left a comment

fmigneault Oct 20, 2021 •

edited

Loading

ChaamC Oct 20, 2021

ChaamC Oct 20, 2021

ChaamC Oct 20, 2021

fmigneault Oct 20, 2021

ChaamC Oct 20, 2021

fmigneault Oct 22, 2021

ChaamC Oct 22, 2021

ChaamC commented Oct 20, 2021

fmigneault commented Oct 20, 2021

fmigneault Oct 26, 2021

fmigneault Oct 26, 2021

ChaamC Oct 26, 2021

fmigneault left a comment

	if not basic_info:
	grp_names = group_names if group_names else [grp.group_name for grp in user.groups]
	user_info["group_names"] = list(sorted(grp_names))

	class User(UserMixin, Base):
	def __str__(self):
	return "<User: name={} id={}>".format(self.user_name, self.id)

Feature add tc #467

Feature add tc #467

Conversation

ChaamC commented Sep 21, 2021 • edited Loading

Features / Changes

Bug Fixes

Other references

codecov-commenter commented Sep 21, 2021 • edited Loading

Codecov Report

fmigneault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaamC Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaamC Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaamC commented Oct 19, 2021

fmigneault left a comment

Choose a reason for hiding this comment

fmigneault Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaamC commented Oct 20, 2021

fmigneault commented Oct 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmigneault left a comment

Choose a reason for hiding this comment

ChaamC commented Sep 21, 2021 •

edited

Loading

codecov-commenter commented Sep 21, 2021 •

edited

Loading

ChaamC Sep 22, 2021 •

edited

Loading

ChaamC Sep 22, 2021 •

edited

Loading

fmigneault Oct 20, 2021 •

edited

Loading