Fix missing password policy definitions #7225
Conversation
Generated by 🚫 dangerJS |
Codecov Report
@@ Coverage Diff @@
## master #7225 +/- ##
==========================================
+ Coverage 93.92% 93.93% +0.01%
==========================================
Files 181 181
Lines 13267 13267
==========================================
+ Hits 12461 12463 +2
+ Misses 806 804 -2
Continue to review full report at Codecov.
|
| 2. If a parameter group (nested parameter) has been added: | ||
| - add the environment variable prefix for the parameter group to `nestedOptionEnvPrefix` in [/resources/buildConfigDefinition.js](https://github.com/parse-community/parse-server/blob/master/resources/buildConfigDefinition.js) | ||
| - add the parameter group type to `nestedOptionTypes` in [/resources/buildConfigDefinition.js](https://github.com/parse-community/parse-server/blob/master/resources/buildConfigDefinition.js) | ||
| 3. Execute `npm run definitions` to automatically create the definitions in [/src/Options/Definitions.js][config-def] and [/src/Options/docs.js][config-docs]. |
There was a problem hiding this comment.
parameter group / group type / nested parameters sounds confusing. I think we should make the user aware of the /resources/buildConfigDefinition.js file. Like if you need to add definition types add them to buildConfigDefinition.js
I want to add a new type NumberOrObject, which file should I use?
There was a problem hiding this comment.
parameter group / group type / nested parameters sounds confusing.
Yes, I struggled myself to find a proper term. A new config parameter only needs to be added to buildConfigDefinition.js if it is a new interface type, otherwise the file doesn't have to be touched. By new interface I mean a parameter that is actually an object that holds sub-parameters, like PagesOptions. What term do you suggest?
I want to add a new type NumberOrObject, which file should I use?
Good question, I think no change required in buildConfigDefinition.js, because it can also be a number, therefore not sure how this would be handled. This may not be a guide that covers all cases, but the reason I added this paragraph some months ago was that contributors often did incorrectly implement new parameters, most often leaving out the buildConfigDefinition.js file. I don't think a simple "if you need to add definition types add them to buildConfigDefinition.js" will cut it.
There was a problem hiding this comment.
To be honest, when I first documented the definitions, I assumed the buildConfigDefinition.js would rarely get updated (4 people have touched it in 4 years, I being the second).
@davimacedo Since you haven't touched the buildConfigDefinition.js do you have any suggestions?
There was a problem hiding this comment.
Maybe we can automate the 2 things that sometimes need to get updated.
There was a problem hiding this comment.
@dplewis I reworded this with an example of an existing parameter. That should make it easier to follow. If you have any wording suggestions, please let know, otherwise we just go ahead and leave this up for improvement in the future. It's an improvement over status quo anyway.
|
@mtrezza Can you resolve the conflicts? Anything else you want to add to the PR? |
|
This looks old, I'll work this over in the next few days, not sure whether all the info is still valid. |
mtrezza
force-pushed
the
fix-missing-password-policy-definitions
branch
from
d5dcf89 to
7fc548f
Compare
|
@dplewis This is ready for review. |
mtrezza
requested review from
davimacedo,
Moumouls and
dplewis
and removed request for
davimacedo and
Moumouls
|
🎉 This change has been released in version 5.0.0-beta.1 |
|
🎉 This change has been released in version 5.0.0 |
New Pull Request Checklist
Issue Description
In #7040 some new parameter definitions in
passwordPolicywere improperly added without specifying default parameters.Related issue: (none; addition to #7017, #7040)
Approach
passwordPolicy.resetTokenReuseIfValidTODOs before merging