-
Notifications
You must be signed in to change notification settings - Fork 79
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
LF-4562 remove unused animal sections #3539
Conversation
Wondering if we can comment out backend stuff too? It might be nice to prevent insertions on group table on all api accessible routes. Just so when we return to it we have a guaranteed clean db. |
@Duncan-Brain yeah for sure, are you referring to the group related routes? I can comment out those. |
Those yes, but it is also possible to add groups on add/edit animals/batches. I am not sure whether the |
…ns-b Fix broken tests by removing groups from eager expression
@@ -422,9 +403,7 @@ export default function AnimalInventory({ | |||
}; | |||
|
|||
const iconActions: iconAction[] = [ | |||
{ label: t(`common:ADD_TO_GROUP`), iconName: 'ADD_ANIMAL', onClick: () => ({}) }, | |||
{ label: t(`common:CREATE_A_TASK`), iconName: 'TASK_CREATION', onClick: () => ({}) }, |
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.
Kept this one because I added the feature on #3564
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.
Has a merge conflict but it looks good to me!
Is there a difference between the tests that were skipped vs the ones deleted?
@kathyavini I deleted all the logic related to groups from the animal/batch controllers and models (I was initially going to comment it out but thought it might end up being too confusing for future readers), and so since that functionality no longer exists in the code I removed the associated tests as well. For groups, I kept the routes, controller and model and so I kept the tests. Not sure if this makes total sense though, I wasn't sure! I could also just skip the test that was entirely removed on the animal test file |
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 since that functionality no longer exists in the code I removed the associated tests as well
That distinction makes sense to me!
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.
Just one thing to revert!
Could we keep the translations? I would feel bad for the translators if they had to translate the same strings again...
packages/webapp/src/App.jsx
Outdated
|
||
function App() { | ||
const [isCompactSideMenu, setIsCompactSideMenu] = useState(false); | ||
const [isFeedbackSurveyOpen, setFeedbackSurveyOpen] = useState(false); | ||
const FULL_WIDTH_ROUTES = ['/map', ANIMALS_INVENTORY_URL, ANIMALS_URL]; | ||
const FULL_WIDTH_ROUTES = ['/map', ANIMALS_INVENTORY_URL]; |
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.
We need ANIMALS_URL
to make the animal details view full width ;)
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.
Good call!!
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.
Awesome thanks for doing this!
Edit: saw Sayaka is also reviewing haha ... undo my approval!
Question: should we immediately make a "revert" PR for future us?
@@ -43,17 +41,15 @@ import { useSelector } from 'react-redux'; | |||
import { locationsSelector } from '../../locationSlice'; | |||
import { Location } from '../../../types'; | |||
|
|||
export type AnimalInventory = { | |||
export type AnimalInventoryItem = { |
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.
Thanks for improving this!
@Duncan-Brain Aren't we doing a sensors release before we add anything to animals, though? @antsgar you have another conflict, sorry. At this point it feels like every merge is a conflict in three other PRs 😅 |
@kathyavini yeah, just asking if we should make a PR -- not merge it. Probably answer is no haha |
@Duncan-Brain it'd be pretty easy to create one with a revert commit from the merge commit once this is merged, so I think we could do that when the time comes! @SayakaOno I readded the translation strings, good call |
Description
Jira link:
https://lite-farm.atlassian.net/browse/LF-4562
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: