Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow scheduling a (example) product via the web UI #5933
Allow scheduling a (example) product via the web UI #5933
Changes from all commits
9628bf4
4ebee25
bf997a4
e805737
ecb52cf
313ee7a
916b53a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I still don't like that we are creating some extra code for this specific feature to handle nested data, while we could do this in a more generic way in Setup.pm with the group feature.
Also it moves any validation we might want to do to runtime instead of server startup.
Since it's something internal we can probably change the implementation later, but just saying.
Not sure what others think.
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.
I tried to keep it simple but chose the section name so the group feature can be used in the future for more generic parsing. So if we needed to parse this in a more generic way at some point it'll be possible without a breaking change to the config file format.
This file was deleted.
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?
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 I had to add config values for testing instead of just testing with the default config. (We want to cover the part of the code that allows the users/admins to override values so we obviously need to supply this kind of configuration when running the test.)
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.
In other tests we just change the config on the fly to test specific features