Add permissions for QA and Relman signoff fixes #1171 #1225

uhlissuh · 2019-04-30T16:16:52Z

This PR adds user groups to Experimenter with special permissions to be able to check or uncheck the QA signoff and Release Management signoff boxes. The groups will be created if they don't exist and permissions added upon app startup. The admin page will need to be used to add specific users to each group. The form now checks that the user has permissions to check QA Signoff and Relman Signoff before the DB is updated.

If a user tries to check a box they don't have permissions for, a message appears on their screen that warns them they can't do it, and the changes are not made to the experiment in the DB. If a user is also checking/unchecking other boxes at the time of form submission in addition to the box they don't have permissions for, the form will submit the permitted changes to the DB but will skip making any changes to the restricted fields. It will warn the user that the action isn't allowed plus inform them of the changes that were successfully made.

Questions that remain for me are:

Styling of two NOT permitted messages happening simultaneously. Right now, I throw up two separate messages, figuring the case will be rare enough that it didn't warrant the extra effort of making them the same message... but I can most certainly fix this if someone thinks I should.
This change broke other tests that involved submitting general fake form data. In some cases it seemed helpful/pertinent to add the permissions to the user for an existing test so that qa/relman signoff data could be submitted in the fake form. In a few cases it seemed not germane to the test at hand, so I just changed the dummy data to other fields that didn't have permissions issues to keep the test simple. Let me know if I ran afoul here anywhere with test changes.

glasserc

Looks OK to me although I see from comments on Slack that you and @mythmon are sorting out the CI failures.

app/experimenter/experiments/apps.py

app/experimenter/templates/base.html

glasserc · 2019-04-30T18:31:20Z

app/experimenter/templates/base.html

@@ -99,6 +99,24 @@ <h3 class="pt-2">
      </div>
    {% endif %}

+    {% if messages %}


Is this really the first use of messages in this project? I thought I had seen other messages elsewhere..

@glasserc Could you elaborate a bit more on what you are thinking here?

It seemed odd to me that we never used messages here, since we seem to have other kinds of messages already in the project. Indeed, it seems like there's a Notification model. Do we need both? Can we stick with the one that already exists in the project?

I didn't use it because I didn't think this message warranted writing to the database since it's just a warning and a lack of actual action. Seems like if you use the Notification class you are writing the change made to the database (like X person added the datascience review) so that changes can be kind of kept track of, displayed in the review history section, and also then send that little blue message. Kinda like a changelog for changes made in the review process. I didn't think "user tried to check something they weren't allowed to check and we didn't save anything" rose to that same level of importance.. Maybe my understanding of Notification is off.

To add on, I actually was intentionally not using it because I didn't want that "tried to check permission but couldnt" going into that review history section, so if I did use Notification I'd have to have some extra logic to fish it out of that section maybe...

The module being used here, django.contrib.messages, is about one-off messages that happen within a request/response cycle, usually feedback about that specific response. The Notification model seems to be longer term notifications that carry a read/unread state, and is set up to tie in to sending emails (which messages wouldn't be good for).

I think that having both systems is good.

That being said, I think the styling of these messages here isn't very good. Since this is the first time we're integrating messages, I think we can make it a bit less confusing. Right now the messages blend into the navigation header too much, and it doesn't distinguish between the levels of the message (like warning, error, or info).

I'd suggest something outside of the usual flow of the page, like a toast:

So @mythmon, are you saying in "I think having both systems is good" that you mean you agree with the decision to use plain messages instead of Notification for this use case?

Also, are you saying you want me to change the styling to this example styling in the PR?

Yes, I think using messages in this case is the right choice.

Since the styling for messages was introduced in this PR, it's in scope to make it look not-confusing. I don't have strong opinions about how it should look, just that the current UI is confusing because it blends in with site navigation. The screenshot I posted is an idea, but it's a rough idea.

I think the best choice here would be to use Bootstrap 4.2's Toast component, but Experimenter uses Bootstrap 4.1, and I don't know how big of an upgrade that would be.

OK, I don't mind changing the styling. Perhaps I'll run past @jaredkerim when he gets back and @shell1 to see what hopes and dreams they would have for the styling of these warning messages?

OK, thanks for explaining. I agree that having both is the right decision.

app/experimenter/experiments/tests/test_forms.py

glasserc

I feel a little unqualified to review the migrations so I'll hold off for now.

app/experimenter/experiments/apps.py

glasserc · 2019-05-01T15:13:54Z

app/experimenter/templates/base.html

@@ -99,6 +99,24 @@ <h3 class="pt-2">
      </div>
    {% endif %}

+    {% if messages %}


It seemed odd to me that we never used messages here, since we seem to have other kinds of messages already in the project. Indeed, it seems like there's a Notification model. Do we need both? Can we stick with the one that already exists in the project?

app/experimenter/experiments/migrations/0050_create_group_add_permissions.py

mythmon · 2019-05-01T16:42:39Z

app/experimenter/templates/base.html

@@ -99,6 +99,24 @@ <h3 class="pt-2">
      </div>
    {% endif %}

+    {% if messages %}


The module being used here, django.contrib.messages, is about one-off messages that happen within a request/response cycle, usually feedback about that specific response. The Notification model seems to be longer term notifications that carry a read/unread state, and is set up to tie in to sending emails (which messages wouldn't be good for).

I think that having both systems is good.

That being said, I think the styling of these messages here isn't very good. Since this is the first time we're integrating messages, I think we can make it a bit less confusing. Right now the messages blend into the navigation header too much, and it doesn't distinguish between the levels of the message (like warning, error, or info).

I'd suggest something outside of the usual flow of the page, like a toast:

#9385) Bumps [django-storages](https://github.com/jschneier/django-storages) from 1.13.1 to 1.14. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/jschneier/django-storages/blob/master/CHANGELOG.rst">django-storages's changelog</a>.</em></p> <blockquote> <p>1.14 (2023-09-04)</p> <hr /> <h2>General</h2> <ul> <li><strong>Breaking</strong>: Drop support for Django 4.0 (<code>[#1235](https://github.com/jschneier/django-storages/issues/1235)</code>_)</li> <li><strong>Breaking</strong>: The long deprecated & removed (from Django) <code>(modified|created|accessed)_time</code> methods have been removed from the various storages, please replace with the <code>get_(modified|created|accessed)_time</code> methods</li> <li>Add support for saving <code>pathlib.PurePath</code> names (<code>[#1278](https://github.com/jschneier/django-storages/issues/1278)</code>_)</li> <li>Add support for Django 4.2 (<code>[#1236](https://github.com/jschneier/django-storages/issues/1236)</code>_)</li> </ul> <h2>Azure</h2> <ul> <li>Set <code>account_(name|key)</code> from <code>connection_string</code> if not provided (<code>[#1225](https://github.com/jschneier/django-storages/issues/1225)</code>_)</li> </ul> <h2>Dropbox</h2> <ul> <li><strong>Deprecated:</strong> The name <code>DropboxStorage.location</code> has been deprecated, please rename to <code>DropboxStorage.root_path</code>, a future version will remove support for the old name. (<code>[#1251](https://github.com/jschneier/django-storages/issues/1251)</code>_)</li> <li>Storage and related names with a captialized B have been changed to no longer have one e.g <code>DropboxStorage</code> has now replaced <code>DropBoxStorage</code>. Aliases have been added so no change is necessary at this time. A future version might deprecate the old names. (<code>[#1250](https://github.com/jschneier/django-storages/issues/1250)</code>_)</li> <li><code>DropboxStorage</code> now conforms to the <code>BaseStorage</code> interface (<code>[#1251](https://github.com/jschneier/django-storages/issues/1251)</code>_)</li> <li>Fix name mangling when saving with certain complex root paths (<code>[#1279](https://github.com/jschneier/django-storages/issues/1279)</code>_)</li> </ul> <h2>FTP</h2> <ul> <li>Use setting <code>BASE_URL</code> if it is defined (<code>[#1238](https://github.com/jschneier/django-storages/issues/1238)</code>_)</li> </ul> <h2>Google Cloud</h2> <ul> <li><strong>Breaking</strong>: Support for the deprecated <code>GS_CACHE_CONTROL</code> has been removed. Please set the <code>cache_control</code> parameter of <code>GS_OBJECT_PARAMETERS</code> instead. (<code>[#1220](https://github.com/jschneier/django-storages/issues/1220)</code>_)</li> </ul> <h2>Libcloud</h2> <ul> <li>Reading a file that does not exist will now raise <code>FileNotFoundError</code> (<code>[#1191](https://github.com/jschneier/django-storages/issues/1191)</code>_)</li> </ul> <h2>SFTP</h2> <ul> <li>Add closing context manager for standalone usage to ensure connections are cleaned up (<code>[#1253](https://github.com/jschneier/django-storages/issues/1253)</code>_)</li> </ul> <h2>S3</h2>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/jschneier/django-storages/commit/d1cd2db41fd3bec01fe97d4b095159f974b91673"><code>d1cd2db</code></a> Release version 1.14 (<a href="https://redirect.github.com/jschneier/django-storages/issues/1291">#1291</a>)</li> <li><a href="https://github.com/jschneier/django-storages/commit/557226a9147545ea17a0f9edfbe5a974c09e1dde"><code>557226a</code></a> ignore s3 namespace change in git blame (<a href="https://redirect.github.com/jschneier/django-storages/issues/1290">#1290</a>)</li> <li><a href="https://github.com/jschneier/django-storages/commit/e1c3b38cc2e6c5bc8330db046386a0541dd1d39e"><code>e1c3b38</code></a> [s3] change s3 backend namespace to s3 and add compat shim (<a href="https://redirect.github.com/jschneier/django-storages/issues/1289">#1289</a>)</li> <li><a href="https://github.com/jschneier/django-storages/commit/c3a567d0aa13d248b9bef652cee8ff3d678f5522"><code>c3a567d</code></a> point to docs for all configuration (<a href="https://redirect.github.com/jschneier/django-storages/issues/1288">#1288</a>)</li> <li><a href="https://github.com/jschneier/django-storages/commit/abce92af42f8d66544146daf8d0a5124f5b576e5"><code>abce92a</code></a> [general] remove dead Storage API methods (<a href="https://redirect.github.com/jschneier/django-storages/issues/1287">#1287</a>)</li> <li><a href="https://github.com/jschneier/django-storages/commit/b02f7e26de45bdbd93f0ce6cd32beb64d4922a06"><code>b02f7e2</code></a> [s3] fix retrieving SSE-C files (<a href="https://redirect.github.com/jschneier/django-storages/issues/1286">#1286</a>)</li> <li><a href="https://github.com/jschneier/django-storages/commit/ad36a9ffd57b94afb051f7bb4715dc706af92b96"><code>ad36a9f</code></a> [docs] document OPTIONS settings (<a href="https://redirect.github.com/jschneier/django-storages/issues/1285">#1285</a>)</li> <li><a href="https://github.com/jschneier/django-storages/commit/19a15c2054ffc90994578fd2a99918ab81218864"><code>19a15c2</code></a> [s3] fix saving files with string content (<a href="https://redirect.github.com/jschneier/django-storages/issues/911">#911</a>)</li> <li><a href="https://github.com/jschneier/django-storages/commit/89ca5bc8e31095f4c9e8bdf851557e41c6364122"><code>89ca5bc</code></a> [s3] add s3 extras_require (<a href="https://redirect.github.com/jschneier/django-storages/issues/1284">#1284</a>)</li> <li><a href="https://github.com/jschneier/django-storages/commit/53d66750dd89e12afd826ca5483c936fe615cfd8"><code>53d6675</code></a> [s3] S3Boto3StorageFile respects mode on readlines (<a href="https://redirect.github.com/jschneier/django-storages/issues/1000">#1000</a>)</li> <li>Additional commits viewable in <a href="https://github.com/jschneier/django-storages/compare/1.13.1...1.14">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=django-storages&package-manager=pip&previous-version=1.13.1&new-version=1.14)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

uhlissuh force-pushed the permissions-QA-relman-checkboxes-1171 branch from c78dc38 to 3fd08dc Compare April 30, 2019 16:55

glasserc approved these changes Apr 30, 2019

View reviewed changes

uhlissuh force-pushed the permissions-QA-relman-checkboxes-1171 branch 4 times, most recently from 0f8d581 to b9d22fa Compare April 30, 2019 21:26

glasserc reviewed May 1, 2019

View reviewed changes

uhlissuh force-pushed the permissions-QA-relman-checkboxes-1171 branch from b9d22fa to e2f41ae Compare May 1, 2019 16:27

mythmon suggested changes May 1, 2019

View reviewed changes

Add permissions for QA and Relman signoff fixes mozilla#1171

98c1b40

uhlissuh force-pushed the permissions-QA-relman-checkboxes-1171 branch from e2f41ae to 98c1b40 Compare May 1, 2019 19:46

mythmon approved these changes May 2, 2019

View reviewed changes

uhlissuh merged commit 9534546 into mozilla:master May 2, 2019

uhlissuh deleted the permissions-QA-relman-checkboxes-1171 branch May 2, 2019 16:51

This was referenced May 2, 2019

Change styling of error messages when user doesn't have permissions #1234

Closed

Fix migration order issue regarding migration numbers 49-53 #1236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add permissions for QA and Relman signoff fixes #1171 #1225

Add permissions for QA and Relman signoff fixes #1171 #1225

uhlissuh commented Apr 30, 2019 •

edited

Loading

glasserc left a comment

glasserc Apr 30, 2019

uhlissuh Apr 30, 2019

glasserc May 1, 2019

uhlissuh May 1, 2019

uhlissuh May 1, 2019

mythmon May 1, 2019

uhlissuh May 1, 2019

mythmon May 1, 2019

uhlissuh May 1, 2019

glasserc May 1, 2019

glasserc left a comment

glasserc May 1, 2019

mythmon May 1, 2019

Add permissions for QA and Relman signoff fixes #1171 #1225

Add permissions for QA and Relman signoff fixes #1171 #1225

Conversation

uhlissuh commented Apr 30, 2019 • edited Loading

glasserc left a 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

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

glasserc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhlissuh commented Apr 30, 2019 •

edited

Loading