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

fix: do not require change password to change reporting configuration #1004

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

jajjibhai008
Copy link
Contributor

@jajjibhai008 jajjibhai008 commented Jul 10, 2023

Description

Problem:
When we try to update a configuration report then the password is required even if I did not want to update it.

Solution:
I make the password field conditionally required while updating a report.

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (a209dd0) 83.24% compared to head (ea6fba3) 83.29%.

❗ Current head ea6fba3 differs from pull request most recent head af86f42. Consider uploading reports for the commit af86f42 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
+ Coverage   83.24%   83.29%   +0.04%     
==========================================
  Files         398      398              
  Lines        8649     8649              
  Branches     1791     1791              
==========================================
+ Hits         7200     7204       +4     
+ Misses       1411     1407       -4     
  Partials       38       38              
Impacted Files Coverage Δ
...components/ReportingConfig/ReportingConfigForm.jsx 85.57% <100.00%> (+3.84%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@bilaltahir21 bilaltahir21 left a comment

Choose a reason for hiding this comment

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

I think it would be better to use the existing variable requiredFields to check for password, it would keep validation logic in one place.

Can we add some tests to check password checkbox scenario? It would be helpful in future if anything in related logic changes.

@jajjibhai008
Copy link
Contributor Author

I think it would be better to use the existing variable requiredFields to check for password, it would keep validation logic in one place.

Can we add some tests to check password checkbox scenario? It would be helpful in future if anything in related logic changes.

I think in our case we are making password and PGP key conditionally required so placing them in required fields will make the logic more complex of making fields conditionally required. So, that is why I choose this way.

I have added a test case of it too.

@jajjibhai008 jajjibhai008 merged commit 8b6d1c8 into master Jul 13, 2023
5 checks passed
@jajjibhai008 jajjibhai008 deleted the eahmadjaved/ENT-7286 branch July 13, 2023 09:43
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.

3 participants