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

Default Pet Tasks: tasks should be assigned to a general category, or a species #709

Closed
kasugaijin opened this issue May 14, 2024 · 14 comments

Comments

@kasugaijin
Copy link
Collaborator

kasugaijin commented May 14, 2024

Currently default pet tasks are assigned to all new pets, regardless of species. This is not as practical as grouping default pet tasks by species. We can also have a general category, that applies to all species.

  • On the Pet model, add an enum at 0 index all
  • In the form used to update/create a pet, do not show the all option in the select
  • Add a new column to the DefaultPetTasks table, species, string, default 0 (the all enum)
  • Add this new attribute to the Default Pet task form as a select showing all enums for pet species
  • Update the default pet task index table in the UI to show species column
  • Organize default pet tasks in the index alphabetically on species
  • Update the default pet task service so that when a new pet is created, it's associated tasks will be the tasks with all AND the same species as the Pet. E.g., A new dog will get tasks with all and dog species.
  • Update tests
@kasugaijin kasugaijin added on hold Further investigation/decision-making is required Ready Make a comment to get assigned. and removed on hold Further investigation/decision-making is required labels May 14, 2024
@atbalaji
Copy link
Contributor

Hi @kasugaijin, I have raised 2 PRs . After they are over and merged, i would like to work on this issue

@kasugaijin
Copy link
Collaborator Author

@atbalaji once both PRs are merged, if this is still not assigned, for sure! But just in case someone else wants it before then we don't hold issues for people to give a fair chance to all to contribute.

@atbalaji
Copy link
Contributor

Hi @kasugaijin , I would like to take this issue up as now both of my PRs are merged

@kasugaijin
Copy link
Collaborator Author

@atbalaji all yours

@kasugaijin kasugaijin added Assigned and removed Ready Make a comment to get assigned. labels May 16, 2024
@atbalaji
Copy link
Contributor

Just for confirmation @kasugaijin Can you elaborate the first point. U mean to create an enum like named 'species' where 0 represents 'all' option ? That's what u mean ?

@kasugaijin
Copy link
Collaborator Author

@atbalaji check the Pet model as there is already an enum. Just need to add an ‘all’ at the zero index of that enum.

@atbalaji
Copy link
Contributor

Hi @kasugaijin .I have made changes except for tests part locally. Some differences with the requirement, we shall discuss during PR. But one thing i noticed during the changes is since default pet tasks are not associated with the tasks they create. On deletion of default pet task the corresponding task of a pet is not deleted. Is this the intended behavior?

@kasugaijin
Copy link
Collaborator Author

Thanks @atbalaji yes that’s fine. The default pet task is a blueprint for pet tasks but does not need to be associated.

@atbalaji
Copy link
Contributor

Hi @kasugaijin . Is it okay to make a PR without tests first. Existing tests are passing after changes. Since my approach differs a little from the suggested one, I thought of writing tests after we agree on common ground after an initial PR.

@kasugaijin
Copy link
Collaborator Author

Yes for sure you can make a PR without tests and add them later

Copy link

github-actions bot commented Jul 9, 2024

This issue is marked as stale due to no activity within 30 days. If no further activity is detected within 7 days, it will be unassigned.

Copy link

Automatically unassigned after 7 days of inactivity.

@wiliajc87
Copy link
Contributor

@kasugaijin I was browsing issues and noticed this one--looks like it could be closed since that PR was merged?

@kasugaijin
Copy link
Collaborator Author

@wiliajc87 thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants