Updated new job composer project validation - Single Project Model

[abujeda]

Fixes #2976

Updated project validation now that project directory can be configured by the user.
Draft PR to showcase approach for new project validation using create and update flows

[abujeda]

I need to improve the tests if this approach goes in the right direction.

[johrstrom]

Seems like we just want FileUtils.remove_dir(configuration_directory, true) and not what's in the if block right? I think deleting the entire directory would be an extra checkbox or similar as it may hold code, data and more.

[abujeda]

I think it makes sense to only delete the configuration_directory to avoid deleting scripts and outputs.
I kept the delete the project_dataroot as it was part of the original code.

An issue with no deleting project_dataroot is that ids are re-used so the same dir could be reused when deleting and creating projects. Which is not a good edge case.

[johrstrom]

An issue with no deleting project_dataroot is that ids are re-used so the same dir could be reused when deleting and creating projects. Which is not a good edge case.

Yea IDs need to change, I just hadn't filed the ticket yet - #2991

[johrstrom]

I like this better than before - reviewing it I can see we're all over the place in terms of who knows what (controller now needing to set_defaults) and how things are done (projects_controller#create has to call projects#new, projects#create and project#save).

I'm going to hack around and see if there's something simpler because there just has to be.

[johrstrom]

Yea the more I look at this the less I like what we're doing. Can I ask that you pick something else up and let me take care of this? Looking through git blame - we did not take great care around the directory and now there's a lot of cruft surrounding it (like project_dataroot - how is that different from self.directory? looks like it's just a bit wrong).

[abujeda]

OK - will look for something else in the backlog

[johrstrom]

OK - will look for something else in the backlog

Sorry for the back and forth - I just needed to do this myself to see what's what. You can continue on this pull request if you like. Here are my comments after doing this myself from scratch (from master)

• ActiveModel validations are great. We should use them instead of the other methods
• I don't care for the additional #create and #set_defaults methods. Indeed I don't think the controller should be setting the id (as it is currently doing now). I put some extra logic in save given we know what that context is. I.e., it is for sure a brand new project, otherwise we'd be calling update.
• I think a big issue we have is in #initialize. I think it should just react correctly given the different contexts (i.e., new project vs. updating an existing one).

Here's the diff I was hacking around on. It has a few more updates than are strictly required but we're sort of doing different things in different spots that we need to clean up.

diff.txt

[abujeda]

I had a look at your approach and made some changes locally for validation.
One thing that does not work with this approach, and the reason I had 2 different set of validations in the PR, is that when a new request comes in. We add the default values, then we validate. On validation errors, we render the form again. In this case, the directory and icon will be populated with the default values (if none provided initially by the user). This is not a good UX. The same will happen if there is a value, then is cleared. The default will come back.

Not quite sure how to fix this issue.
This is why I had initially the project_request object and then the :create and :update validations

[johrstrom]

In this case, the directory and icon will be populated with the default values (if none provided initially by the user). This is not a good UX. The same will happen if there is a value, then is cleared. The default will come back.

Yea I noticed that too. I think we can set @dir=nil in the #save's else block (likely set the id back to nil as well) or in an https://apidock.com/rails/ActiveModel/Validations/Callbacks/ClassMethods/after_validation ?

[abujeda]

I have made some changes based on your patch.
Still using validation based on the method that was called.
This could be replaced with conditional validation based on the new method: new_record?

[johrstrom]

IDK what your dev environment looks like - but you should use solargraph (I use the extension for VS Code).

It'll tell you simple mistakes like this:

[abujeda]

Fixed.

I use RubyMine. Will check if this plugin, or similar, is available

[johrstrom]

yea really it's just a rubocop rule, solargraph is just the language server. Looks like jetbrains has support for it.

There's a .rubocop.yml at the top level of this project with a rake task to copy it to a more local directory (if you're like me and just open the apps/dashboard directory instead of the entire project).

[abujeda]

@johrstrom if the general approach for the PR is going in the right direction, I can continue working on the tests.

I haven't fix them as I wasn't sure you were ok with the solution.

[johrstrom]

@johrstrom if the general approach for the PR is going in the right direction, I can continue working on the tests.

I haven't fix them as I wasn't sure you were ok with the solution.

Sorry I didn't realize you were held up by me. Yes I think this is the right approach.

[johrstrom]

Works for me, thanks!