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

add confirmation flash when submission preferences are saved #5046

Merged
merged 6 commits into from
Dec 18, 2019
Merged

add confirmation flash when submission preferences are saved #5046

merged 6 commits into from
Dec 18, 2019

Conversation

petevdp
Copy link
Contributor

@petevdp petevdp commented Nov 29, 2019

Status

Ready for review

Description of Changes

Fixes #5009.

Changes proposed in this pull request:

Add a flashed message when a user succesfully submits a change with the submission preferences form. The current implementation doesn't differentiate between submissions which actually change anything, but that should be easy enough if it would be desirable.

Testing

manual

  • navigate to /admin/config
  • choose a submission preference by checking/unchecking box
  • hit 'update submission preferences'

A flashed message should show up, indicating that the setting has been successfully saved.

automated

I'm unsure if this feature needs automated testing. I'll be happy to add some if required!

Checklist

If you made changes to the server application code:

  • [x ] Linting (make lint) and tests (make test) pass in the development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

Unsure if I need to do this, as mentioned above.

@eloquence
Copy link
Member

Hi @petevdp, thanks for working on this! From my end, except for the two nits noted above, this looks good. Please give us a few days and a maintainer should be able to have a closer look.

From a UX perspective, it's a bit problematic that the confirmation only is noticeable when you scroll all the way to the bottom of the page:
Screenshot from 2019-12-03 17-41-31

That said, this in-line placement is consistent with how we handle logo updates on the same page. We may want to revisit that separately but I'd personally be OK with doing this in follow-up. CC @ninavizz for UX visibility.

@petevdp
Copy link
Contributor Author

petevdp commented Dec 5, 2019

@eloquence Yeah it seems like we might want to add a more generalized system for the sort of confirmation/notification system that would be good for situations like this. Maybe something JS based, or a redirect to another page with the desired message.

That sounds like it might justify creating a separate issue, but I'd still like to work on that if i can.

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

looks good! a few comments inline

@@ -0,0 +1,10 @@
{# these are flashed messages for the logo upload file verifiaction #}
Copy link
Contributor

Choose a reason for hiding this comment

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

let's update or remove this comment (these are the flashed messages for the submission file preferences)

securedrop/journalist_app/admin.py Show resolved Hide resolved
@eloquence
Copy link
Member

Hi @petevdp, just touching base - do you have time to poke further at this? If not we can finish it up. Just let us know :)

@petevdp
Copy link
Contributor Author

petevdp commented Dec 17, 2019

@eloquence Yeah I do, sorry for the lack of communication. I'm planning on working on it tomorrow!

@eloquence
Copy link
Member

Awesome, thanks for the update! :)

@zenmonkeykstop
Copy link
Contributor

Thanks for the update @petevdp - pushed little a commit to fix linter errors, will review once CI is passing.

Copy link
Contributor

@zenmonkeykstop zenmonkeykstop left a comment

Choose a reason for hiding this comment

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

Tested according to plan, working correctly. Automated tests also passing, so LGTM

@zenmonkeykstop zenmonkeykstop merged commit 4324582 into freedomofpress:develop Dec 18, 2019
@petevdp
Copy link
Contributor Author

petevdp commented Dec 18, 2019

Awesome! Thanks for everyone's help, and sorry for the delay.
@zenmonkeykstop Apologies for the linting errors, definitely for got to run the linter 😅

@DrGFreeman
Copy link
Contributor

Hi @petevdp, kudos for your contribution (got an email notification as I had commented on the issue).

As a new contributor to the project, I found this pre-commit hook quite useful to avoid forgetting the linting checks.

@petevdp
Copy link
Contributor Author

petevdp commented Dec 19, 2019

@DrGFreeman Okay I'll install that, thanks!

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

Successfully merging this pull request may close these issues.

Add confirmation message after updating submission preferences
5 participants