-
Notifications
You must be signed in to change notification settings - Fork 15
New req #1551
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
New req #1551
Conversation
… int instead of string. We worked with bonnerManagement.js in creating a new ajax and a new function to save the row instead of all rows.
… is somehow not defined. commented out code under document.ready() in bonnerManagement.js like ajax. Created a new save requirement function in certification.py.
…Team/celts into bonner_Req_SaveBtn
…wareDevTeam/celts into bonner_Req_SaveBtn
…using jquery, worked on making changes to our route by uncommenting old code in order for our new row to save, and are currently working in certification.py to fix our issue of not saving.
…to work with old rows, and fixed functionality of updateCertRequirements to handle old rows.
…uplication issue, cleaned up and short code created.
This reverts commit 76898e6.
| transaction.rollback() | ||
|
|
||
| @pytest.mark.integration | ||
| def test_updateCertRequirements(): |
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.
Why was this entire test removed?
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.
Because the function that was being tested has been changed and is not fruitful anymore
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.
But the function still exists. It still needs a test. Also, there is no test of your new delete function
| transaction.rollback() | ||
|
|
||
| @pytest.mark.integration | ||
| def test_updateCertRequirements(): |
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.
But the function still exists. It still needs a test. Also, there is no test of your new delete function
| if deleteReq == True: | ||
| return jsonify({"success": True}) | ||
| else: | ||
| return jsonify({"success": False, "message": "Failed to delete requirement."}) |
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.
No success: True or success: False please. Use statuses for failure where possible
In addition, if they ask to delete something and it doesn't exist, who cares? They didn't want it in the first place. That should still count as succeeding.
| data: JSON.stringify(rowid), | ||
| success: function(response) { | ||
| msgToast("Bonner", "Removing Bonner Requirement"); | ||
| location.reload(); |
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.
Reloading is fine, but it would probably be cleaner to just delete the row from the table.
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.
When we remove the reload, the row gets automatically deleted, but it doesn't display until we manually reload. Should we still delete the reload or keep it?
|
Implementation in this PR has grown too complex from the initially simple issue. To solve this, simply prevent saving with an empty frequency |
We should not be able to save a Bonner Requirement with an empty frequency. If we want it to be optional, lets make that clear. Issue with trying to modify the requirement section, along with Dr. Heggen, not liking the general functionality of how we save the rows and add them.
Fixes #1468
Issue with selecting the requirement section in Bonner management, clearing up misunderstanding of allowing frequency to be empty, issues with the process of saving and creating rows
Changes
Testing
(If while trying to see our changes, you notice that the buttons on the bonner management page are not working then it is probably an issue with the symlink. You can solve that on your side by removing sortable.min.js and then doing the command ln -s sortable-1.15.0.min.js sortable.min.js in the VSCode terminal.