[DRAFT] Enable ROR-Based Org Creation for Funders#1262
[DRAFT] Enable ROR-Based Org Creation for Funders#1262momo3404 wants to merge 10 commits intomomo/issues/926-editfrom
Conversation
- Remove org_id setting since it was incorrectly setting org_id
Generated by 🚫 Danger |
aaronskiba
left a comment
aaronskiba
left a comment
There was a problem hiding this comment.
I tried updating an existing plan without touching the funder field. The update seems to have saved successfully, but I got the following error:
aaronskiba
left a comment
aaronskiba
left a comment
There was a problem hiding this comment.
I'm not sure why an org dropdown was never implemented for plan.funder selection. Maybe this would be the right time to do it? Since we are limiting the options to db + ROR orgs, rendering the org names like we are for the contributor affiliation could really help user experience here.
My bad, I just had to precompile the assets. That's awesome that switching to the combined partial adds the dropdown. |
|
Maybe just update the description explaining why these changes are necessary in the controller action. |
| # PUT /plans/1 | ||
| # rubocop:disable Metrics/MethodLength, Metrics/PerceivedComplexity | ||
| def update | ||
| def update # rubocop:disable Metrics/CyclomaticComplexity |
There was a problem hiding this comment.
This update action is already a bit of a monster. Maybe a private helper for the new funder assignment logic will at least prevent the new CyclomaticComplexity offense.
There was a problem hiding this comment.
I added a helper for the funder assignment logic, but that doesn't seem to remove the error. It looks like that was added due to adding !invalid_funder to this line if @plan.update(attrs) && !invalid_funder.
aaronskiba
left a comment
aaronskiba
left a comment
There was a problem hiding this comment.
The above flash message is sometimes rendered even when the plan is in fact saved. For example, I updated a few fields within the "Project Details" tab and added an invalid funder. The other fields were saved, but that flash message was still rendered.
There's another behaviour that may be worth addressing too; what to do when the plan.funder field has a valid entry saved and then the user attempts to save an invalid funder. Currently, when this happens, plan.funder is saved with an empty value. I think it would be better to preserve the previous valid entry in this case.
I just pushed a commit that changes the message to just "Invalid funder" when the funder fails to save, and I edited the funder id assignment to only occur when the field is cleared or there's a valid funder. |
- Change handling to check for if plan updated first, then append invalid funder accordingly - Simplify some of the code for failure messages and alerts
| attrs = remove_org_selection_params(params_in: attrs) | ||
|
|
||
| if @plan.update(attrs) # _attributes(attrs) | ||
| if @plan.update(attrs) |
There was a problem hiding this comment.
I kinda worry about how many lines of code are being changed for this flash message handling.
I wonder if most of this could be reverted and we could simply have something like the following:
if @plan.update(attrs)
notice = success_message(@plan, _('saved'))
notice += _(" The funder was not updated because it is invalid") unless valid_funder
(... previous code)2 participants
Changes proposed in this PR:
updatefunction of thePlansController, so the following changes are made there:- Set funder id in the plan only if a funder is available.
- Add the
invalid_funder?helper function, which returns true when a funder organization is entered and is invalid (does not have a valid ROR or exist in the local DB).- Edit the failure message to include 'Invalid funder' when an invalid funder is entered.