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
Feature/deseng439: Created Timline Widget and merged with Content from Main #2347
Feature/deseng439: Created Timline Widget and merged with Content from Main #2347
Changes from all commits
8df9d31
1e6dc2b
a3af416
0bd8e02
236ef36
579f775
deef67a
92cebe7
afcbaa7
9253268
c1b953c
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 imagine this example text should be changed? CAC forms are unrelated to timelines, right?
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.
So, in met-web, for the widget type ENUM, value 8 is reserved for CAC forms. Timeline is 9. When I looked in the DB, CAC Form was missing as a widget type, so I added it as number 8 and added Timeline as number 9. I thought it would be better to leave that path open, rather than remove all instances of CAC Form.
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.
There seems to be a pattern throughout the app of models only containing some CRUD methods. In looking through the models, some have all 4 methods (GET, PATCH, POST, DELETE) and sometimes half of those are handled in the Service class (including the corresponding DB operations).
I don't understand why that pattern is in place but I feel all CRUD operations should live inside the models. Was there a reason that you didn't include the POST operation besides just copying from another file? I remember you mentioned the DELETE operation is handled by the base Widget class, so I understand that.
I know we have pressure on us to get features done but I also feel the PO would understand us taking more time to make things more consistent and better in the code base.
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.
So anything that touches the db is defined in the model. Then the service just uses those methods from the model to do stuff. I was confused at first too, and so was Nat, but after looking at the logic, I now understand it. Sometimes a service method doesn't need to call a model method that touches the db, so nothing is defined in the model.