Warnings on directory entries #538

chigby · 2018-09-10T14:32:29Z

This pull request:

Adds a warning_level method to the ScanResult class. This returns a string indicating the severity of the warning to be displayed on the corresponding Directory Entry page (or the string "none" if no warning is warranted).
Adds some logic to the DirectoryEntry templates to display the correct warning if the most recent scan result has a warning level. This is a little hairy, and maybe there's some better template-wizardry that could be brought to bear here.
Adds styles to the warnings in accordance with the mock ups on the ticket. This also potentially could be more efficient, but the result does work pretty well when displayed.
Only displays warnings if the directory page is being viewed with warnings=1 in the GET parameters.
Adds a strings.py file to hold the actual warning messages. Each message is keyed to a particular ScanResult attribute that is not what it should be. These messages are descriptive placeholders invented by me and we may not want to use them as the final wording.
Adds tests for the above functionality and makes some incidental improvements to the factories that made it easier for me to write such tests. Also some improvements to the createdirectory command for making fake directory data for better manual testing.

Not finished yet: whitelisting warnings on a per-page basis. Need to get info from @eloquence on his ideas for doing this nicely.

To test this: check out this branch and configure some scan results for testing the various warning levels (or use the manage.py createdirectory command) and make sure everything works as expected.

Fixes #514

This helps in the creation of test and development data. The list of what makes a "moderate" or "severe" warning is not final, just something to get started.

This is helpful for testing the warnings, or other future features of the Directory Entry pages that depend on ScanResults.

Directory entry pages now display a warning if the `warnings=1` query string is present in the URL. The warning text/image depends on the level of the warning (exact logic TBD). Includes tests.

At some point this association was changed to a foreign key from whatever it was before that necessitated saving the association in this way. This should restore the correct behavior and ensure any future code that needs it works, such as the tests for the code in this branch. See commit a0912c6

These are (mostly) placeholders for slightly more informative messages, but I wanted them to be somewhat flexible and be able to reference attributes on (or derived from) the DirectoryEntry or ScanResult objects. Thus I think it's better to have them be stored as python values rather than as part of the page templates.

chigby · 2018-09-10T22:32:09Z

@harrislapiroff Should be ready to review, now. 😄

harrislapiroff

This branch is off to a great start and as far as my manual testing took me, everything works as advertised. Many warnings ⚠️☢️🌈✨

I've got a bunch of minor tweaks to request, but I think we're close!

harrislapiroff · 2018-09-11T17:39:41Z

client/common/sass/directory/_instance-alert.sass

 	padding-left: $icon-width + $icon-gutter + $alert-padding-x
 	border-radius: 4px

+.instance-alert-moderate


It looks like this should be a modifier, so in BEM the syntax for that is more like .instance-alert--moderate

harrislapiroff · 2018-09-11T17:39:57Z

client/common/sass/directory/_instance-alert.sass

+		color: #BAA400
+		font-weight: bold
+
+.instance-alert-severe


.instance-alert--severe

harrislapiroff · 2018-09-11T17:40:55Z

client/common/sass/directory/_instance-instructions.sass

@@ -11,6 +11,8 @@

 	&__landing-page-button
 		+button-small
+
+		$root: &


I had no idea you could do this! 😲

I'll also say I learned about it specifically for this, because the alternative was so verbose I thought "there must be a better way!"

harrislapiroff · 2018-09-11T17:48:53Z

directory/tests/factories/entry.py

@@ -74,3 +75,47 @@ def topics(self, create, count):
 class ScanResultFactory(factory.Factory):
    class Meta:
        model = ScanResult
+
+    class Params:


This is also cool and I did not know about it

harrislapiroff · 2018-09-12T15:44:35Z

directory/templates/directory/_instance_instructions.html

+			</div>
+		</div>
+	{% endif %}
+	<br>


No need for this extra vertical space, I think.

harrislapiroff · 2018-09-12T15:55:25Z

client/common/sass/directory/_instance-instructions.sass

+			text-align: left
+			padding-right: 3rem
+			position: relative
+			color: #BAA400


Let's make the text here the same darker color as the instance alert, while keeping the button border color the same.

harrislapiroff · 2018-09-12T15:56:05Z

client/common/sass/directory/_instance-alert.sass

 	padding-left: $icon-width + $icon-gutter + $alert-padding-x
 	border-radius: 4px

+.instance-alert-moderate
+	color: #BAA400


Let's make the text here darker (maybe 10% to 15%), for legibility, while keeping the icon color the same.

harrislapiroff · 2018-09-12T15:56:30Z

client/common/sass/directory/_instance-alert.sass

+.instance-alert-moderate
+	color: #BAA400
+	a
+		color: #BAA400


same as above

harrislapiroff · 2018-09-12T15:58:56Z

directory/models/entry.py

+        context = super(DirectoryEntry, self).get_context(request)
+        context['show_warnings'] = request.GET.get('warnings') == '1'
+        if context['show_warnings']:
+            context['warning_level'] = self.get_live_result().warning_level(self.warnings_ignored)


Just noting for the future—I don't want it as part of this PR: it would be nice to eventually do things like this using a feature flag library. I was pretty impressed by CFPB's wagtail-flags. Might be worth looking into adding that sometime soon.

harrislapiroff · 2018-09-12T16:01:04Z

directory/models/entry.py

+
+    def formfield(self, **kwargs):
+        defaults = {
+            'form_class': forms.MultipleChoiceField,


How hard would it be to make this widget a CheckboxSelectMultiple instead, to avoid the shift-click ctrl-click UI?

harrislapiroff

I MEANT TO REQUEST CHANGES DO NOT MERGE 😳

chigby · 2018-09-17T16:37:26Z

This PR should be ready to re-review. The changes I've made address the issues that @harrislapiroff except for the CheckboxSelectMultiple widget. That's an easy change to make, but the result is less than optimal with wagtail's default styling:

It would require massaging the wagtail admin CSS to make this more user friendly.

harrislapiroff

One minor text change, but otherwise looks good! @chigby feel free to make that change and merge.

harrislapiroff · 2018-09-18T02:50:09Z

directory/models/entry.py

+                if attribute == 'subdomain' and result.subdomain is True:
+                    warnings.append(
+                        message.format(
+                            'This secure drop landing page',


secure drop => SecureDrop

The text here is a little awkward in general (in particular the repetition of This SecureDrop landing page, but let's merge this ASAP and smooth that over when we get closer to making this a live feature.

I agree it's awkward with the repetition, but the okay thing about this is right now we do have an escape hatch since that phrase is potentially variable. For example we could replace it with "it" or something for each warning after the first. I didn't want to spend too much time on finding the best wording without talking to someone else, though.

Here I'm attempting to match the mock-ups that were made for this feature. My other goal was not to touch the style code too much, for fear of breaking something somewhere else. I think it's pretty close. One thing that remains to be done is update the link to the landing page itself to be the color of the warnings.

Again, mostly what we're doing here is implementing the design.

`title` has a unique constraint in our DB, so we're relying on the randomness of Faker's sentence generator not to create duplicates. With `django_get_or_create` though, we can avoid the problem by not creating a new row if a duplicate name happens to be generated.

I'm adding this field to get around what I belive is a UI deficiency within wagtail, namely that it's not easy to select multiple choices if it's not some kind of many-to-many model relationship. Sometimes you just want a choice field where the person can pick multiple options, and those options are stored in a postgresql array. The default UI for that is a single line, comma separated entry, which I think this ChoiceArrayField is superior to. Added in preparation for choosing warnings to ignore on directory entry pages.

We are hard-coding the list of warnings but letting the admin pick multiple choices. I think this is the nicest user experience: one can easily turn on or off warnings and does not have to remember or look up the names of the fields affected.

If a directory page is ignoring warnings, then: 1. Its warning level is potentially lessened 2. The message corresponding to that warning should not be shown

Some other minor adjustments to CSS for legibility and BEM conformance also included in this changeset.

chigby requested a review from harrislapiroff as a code owner September 10, 2018 14:32

chigby added 7 commits September 10, 2018 10:44

Add ScanResult traits for our warning levels to the factories

ffb7162

This helps in the creation of test and development data. The list of what makes a "moderate" or "severe" warning is not final, just something to get started.

Add fake ScanResults to the createdirectory command

9fb38f8

This is helpful for testing the warnings, or other future features of the Directory Entry pages that depend on ScanResults.

Initial directory entry warnings

586558f

Directory entry pages now display a warning if the `warnings=1` query string is present in the URL. The warning text/image depends on the level of the warning (exact logic TBD). Includes tests.

Use correct logic to determine warning level for a ScanResult

6ae8e86

Fix failing test

43b8846

chigby force-pushed the 514-warnings-on-directory-entries branch from d9e9c99 to 06f76b8 Compare September 10, 2018 14:44

harrislapiroff approved these changes Sep 12, 2018

View reviewed changes

harrislapiroff requested changes Sep 12, 2018

View reviewed changes

harrislapiroff approved these changes Sep 18, 2018

View reviewed changes

chigby added 8 commits September 18, 2018 09:28

Improve styles of landing page link button

9a6b53d

Again, mostly what we're doing here is implementing the design.

Modify warning-level logic to account for ignored warnings

5515d7b

If a directory page is ignoring warnings, then: 1. Its warning level is potentially lessened 2. The message corresponding to that warning should not be shown

Remove unnecessary whitespace

2d953cd

Align warning icon outside main text flow

f540dba

Some other minor adjustments to CSS for legibility and BEM conformance also included in this changeset.

chigby force-pushed the 514-warnings-on-directory-entries branch from be66ce0 to f540dba Compare September 18, 2018 13:28

chigby merged commit 445e7bb into master Sep 18, 2018

chigby deleted the 514-warnings-on-directory-entries branch September 18, 2018 14:08

eloquence mentioned this pull request Sep 26, 2018

[scanner integration] Create per-request flag for showing landing page warnings #498

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warnings on directory entries #538

Warnings on directory entries #538

chigby commented Sep 10, 2018

chigby commented Sep 10, 2018

harrislapiroff left a comment

harrislapiroff Sep 11, 2018

harrislapiroff Sep 11, 2018

harrislapiroff Sep 11, 2018

chigby Sep 12, 2018

harrislapiroff Sep 11, 2018

harrislapiroff Sep 12, 2018

harrislapiroff Sep 12, 2018

harrislapiroff Sep 12, 2018

harrislapiroff Sep 12, 2018

harrislapiroff Sep 12, 2018 •

edited

Loading

harrislapiroff Sep 12, 2018

harrislapiroff left a comment

chigby commented Sep 17, 2018

harrislapiroff left a comment

harrislapiroff Sep 18, 2018

harrislapiroff Sep 18, 2018

chigby Sep 18, 2018

Warnings on directory entries #538

Warnings on directory entries #538

Conversation

chigby commented Sep 10, 2018

chigby commented Sep 10, 2018

harrislapiroff 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

harrislapiroff Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrislapiroff left a comment

Choose a reason for hiding this comment

chigby commented Sep 17, 2018

harrislapiroff 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

harrislapiroff Sep 12, 2018 •

edited

Loading