-
-
Notifications
You must be signed in to change notification settings - Fork 482
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 mandatory fields for partner profile #3691
add mandatory fields for partner profile #3691
Conversation
cf56c5c
to
00d5c72
Compare
00d5c72
to
cfc0944
Compare
@fchatterji I don't have the cycles to do a proper review tonight, but looking at the issue (which I wrote over a year ago and have learned so much since) -- I need to make sure that we are paying attention to the difference between saving the profile and submitting it for approval -- that the fields are not mandatory until being submitted for approval. |
Hey @cielf, well this branch is based off #3651, so it uses the same logic: the validation is checked at save and at approval. So the same as the social network validation on #3651. We could change it, but it's maybe better to first change it on the other branch? That way I can rebase off it, and we avoid conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Unfortunately we can't merge it until my PR is also merged. :(
@dorner which PR is this blocked on? |
Oh #3651, which is merged! |
So there's no reason we know of not to merge this? (We seem to be dragging our collective feet on it). (Though I already did the testing for tomorrow's release) |
Going ahead and merging it. |
Unfortunately this validates for all record saves, not limited to new partner applications. This caused issues with HE Banks that then couldn't edit Partner Agency Information without filling in the now-manditory fields (and surprised them). I think the intention was to only apply this validation of required fields during the initial application? In any case, I'm going to hopefully-temporarily revert this so that the Banks are unblocked. I'll open a new issue to refresh this and maybe apply the rules only at the time of application (ex using AR validation context). |
Resolves #2979
Description
Added mandatory fields to the partner profile model, only on validation context edit
Added tests and fixed failing test
Branch is based off #3651, to take advantage of the new validation context
Type of change
How Has This Been Tested?
Added tests
Screenshots