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

Move validation to models / change forms to embed submodels. #24

Open
NatTuck opened this issue May 5, 2017 · 7 comments
Open

Move validation to models / change forms to embed submodels. #24

NatTuck opened this issue May 5, 2017 · 7 comments
Assignees

Comments

@NatTuck
Copy link
Collaborator

NatTuck commented May 5, 2017

I should be able to delete more than half the controller code.

@blerner
Copy link
Collaborator

blerner commented Jul 15, 2017

Is this now basically done, to your satisfaction?

@NatTuck
Copy link
Collaborator Author

NatTuck commented Jul 15, 2017

There's still a couple cases of obj.save && obj.save_more that should get looked at.

@blerner
Copy link
Collaborator

blerner commented Jul 15, 2017

Iirc, the 'save_upload and save' pattern is because those two operations can logically be done independently of each other, and the upload has to be saved before the main record makes any sense to save...

@NatTuck
Copy link
Collaborator Author

NatTuck commented Jul 16, 2017

If you have a nested association, then the saving the parent model will save the nested records as well atomically, which usually the pattern we want.

@blerner
Copy link
Collaborator

blerner commented Jul 16, 2017

For some reason I recall that not working. I don't remember the details, though. I think it's because the has_one versus belongs_to relationships don't work in our favor here.

@NatTuck
Copy link
Collaborator Author

NatTuck commented Jul 16, 2017

I got it working in for backwards belongs_to for the course form. You need the "accepts_nested_attributes_for" thing in the model and to get the .permit thing right in the controller.

@blerner
Copy link
Collaborator

blerner commented Sep 20, 2018

These have been mostly eliminated. The only places they remain are for submissions (where we pass along a prof_override flag sometimes; this could probably be replaced by a @prof_overrides field) and registrations / registration_requests, where logically multiple things have to be saved together, and we don't have record ids yet for all of them so they can't quite all go into a single save method.

I've forced this to work for assignment uploads, where starter-file uploads now need to know their assignment id, and assignments need to know their starter-file uploads, and this kind of cyclic dependency is a pain to deal with. (This was the major sticking point for save_upload && save pattern.) But the benefits outweighed the downsides: now, all uploads related to a course are in one directory; all uploads related to a single assignment are within a subdirectory of that.

The accepts_nested_attributes_for is a red herring for the actual difficulties here.

blerner added a commit that referenced this issue Sep 9, 2019
Trying to edit weights for all assignments breaks the current code an…
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

No branches or pull requests

2 participants