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

Updated new job composer project validation #2982

Closed
wants to merge 6 commits into from

Conversation

abujeda
Copy link
Contributor

@abujeda abujeda commented Aug 23, 2023

Fixes #2976

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

@abujeda
Copy link
Contributor Author

abujeda commented Aug 23, 2023

This is a demo of the new validation:

project_validation

@abujeda
Copy link
Contributor Author

abujeda commented Aug 23, 2023

I am looking into a different approach to simplify the project model.

@johrstrom
Copy link
Contributor

I am absolutely swamped with OSC work for the start of the semester. I may not be able to look at this in full until next week or later.

@@ -0,0 +1,47 @@
# OodAppLink is a representation of an HTML link to an OodApp.
class ProjectRequest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is a good idea to have another class that represents some intermediate state (i.e., project that hasn't been created yet).

Seems like the model should just validate itself before creating side affects (making a directory and so on).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to use a single project model because of the default values on creation for the icon and directory and the default relationship between id and directory.

In my previous solution, using only the project model, I had to create 2 different ways of saving a project. One for the create flow where id, icon and directory are optional and another for the update flow where all should be populated. I found this solution more complex and difficult to understand.

I can revert back to this solution if you prefer to have a single model.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to look this up because I wondered how ActiveRecord does this. Clearly they carry a bit of state in every object, whether it's been saved and so on. Turns out they keep a member variable @new_record.

https://github.com/rails/rails/blob/72b356cdf56b72d36f287b4a72c655607867475e/activerecord/lib/active_record/persistence.rb#L1220

My point being that this is not a new problem to solve surely there must be patterns/things we can do in a single class.

I can revert back to this solution if you prefer to have a single model.

Yes please, you can create another pull request if you prefer to have both to compare.

In my previous solution, using only the project model, I had to create 2 different ways of saving a project. One for the create flow where id, icon and directory are optional and another for the update flow where all should be populated. I found this solution more complex and difficult to understand.

I see what you mean here - well clearly something needs to change. I don't necessarily mind two different flows for create and update. (we already have save and update)

@id = attributes.delete(:id)
@directory = attributes.delete(:directory)
@directory = Project.dataroot.join(id.to_s).to_s if @directory.blank?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - will revert to the previous solution.

@johrstrom
Copy link
Contributor

I'm going to close this because #2990 is preferable.

@johrstrom johrstrom closed this Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add some safety around where projects can be made
3 participants