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

Protect staff name & phone fields in FMS front-end. #3805

Merged
merged 1 commit into from Mar 16, 2022

Conversation

ludovic-tc
Copy link
Contributor

Name & phone fields for all staff are now disabled in the reporting and updating pages.

fixes mysociety/societyworks#2133

Please check the following:

  • Whether this PR should include changes to any documentation, or the FAQ;
  • Is new functionality tested? CodeCov will warn you about the diff coverage, but won’t complain about e.g. new files;
  • Are the changes tested for accessibility?
  • Have you updated the changelog? If this is not necessary, put square brackets around this: skip changelog

Please check the contributing docs, and describe your pull request here.
Screenshots or GIF animations (using e.g. LICEcap) may be helpful.

Please include any issues that are fixed, using "fixes" or "closes" so that
they are auto-closed when the PR is merged.

Thanks for contributing!

@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #3805 (7def4e4) into master (5a63649) will increase coverage by 0.19%.
The diff coverage is 77.77%.

❗ Current head 7def4e4 differs from pull request most recent head 95ec4c9. Consider uploading reports for the commit 95ec4c9 to get more accurate results

@@            Coverage Diff             @@
##           master    #3805      +/-   ##
==========================================
+ Coverage   82.58%   82.78%   +0.19%     
==========================================
  Files         353      352       -1     
  Lines       24278    24175     -103     
  Branches     3678     3676       -2     
==========================================
- Hits        20050    20013      -37     
+ Misses       3080     3014      -66     
  Partials     1148     1148              
Impacted Files Coverage Δ
web/cobrands/fixmystreet/staff.js 56.41% <77.77%> (+0.45%) ⬆️
web/cobrands/shropshire/assets.js 54.05% <0.00%> (-5.41%) ⬇️
perllib/FixMyStreet/Cobrand/Peterborough.pm 90.43% <0.00%> (-0.18%) ⬇️
web/cobrands/fixmystreet-uk-councils/js.js 100.00% <0.00%> (ø)
web/cobrands/shropshire/cookies.js
web/cobrands/fixmystreet/fixmystreet.js 77.18% <0.00%> (+0.05%) ⬆️
perllib/FixMyStreet/Cobrand/Merton.pm 75.00% <0.00%> (+0.39%) ⬆️
web/js/front.js 83.87% <0.00%> (+1.61%) ⬆️
web/cobrands/buckinghamshire/assets.js 61.62% <0.00%> (+2.28%) ⬆️
web/cobrands/oxfordshire/assets.js 54.44% <0.00%> (+6.94%) ⬆️
... and 1 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 5a63649...95ec4c9. Read the comment docs.

Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

This is a good start, but it e.g. breaks the "another user" option, and the test seems perhaps over-complicated for its needs.

.cypress/cypress/integration/staff.js Outdated Show resolved Hide resolved
.cypress/cypress/integration/staff.js Outdated Show resolved Hide resolved
web/cobrands/fixmystreet/staff.js Outdated Show resolved Hide resolved
Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

Thanks for trying to simplify the test; it does not need to have special exception handling, please see my comment there. The fixes in staff.js look to have been copied/pasted leading to duplicate/incorrect val() calls, and the new JS to call the staff set up won't work if you're not logged in as staff.

web/cobrands/fixmystreet/staff.js Outdated Show resolved Hide resolved
web/cobrands/fixmystreet/staff.js Outdated Show resolved Hide resolved
web/cobrands/fixmystreet/staff.js Outdated Show resolved Hide resolved
.cypress/cypress/integration/staff.js Outdated Show resolved Hide resolved
web/cobrands/fixmystreet/fixmystreet.js Outdated Show resolved Hide resolved
@ludovic-tc ludovic-tc requested review from dracos and removed request for dracos March 10, 2022 15:24
@ludovic-tc ludovic-tc requested a review from dracos March 10, 2022 17:14
@dracos dracos force-pushed the pboro-protect-staff-fields branch from 9685714 to 95ec4c9 Compare March 15, 2022 16:56
@dracos dracos merged commit 95ec4c9 into master Mar 16, 2022
@github-pages github-pages bot temporarily deployed to github-pages March 16, 2022 09:19 Inactive
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.

None yet

2 participants