pet-rescue icon indicating copy to clipboard operation
pet-rescue copied to clipboard

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

Open kasugaijin opened this issue 1 year ago • 12 comments

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 avatar May 14 '24 15:05 kasugaijin

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

atbalaji avatar May 15 '24 15:05 atbalaji

@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.

kasugaijin avatar May 15 '24 15:05 kasugaijin

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

atbalaji avatar May 16 '24 05:05 atbalaji

@atbalaji all yours

kasugaijin avatar May 16 '24 12:05 kasugaijin

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 ?

atbalaji avatar May 21 '24 12:05 atbalaji

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

kasugaijin avatar May 21 '24 13:05 kasugaijin

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?

atbalaji avatar May 29 '24 08:05 atbalaji

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

kasugaijin avatar May 29 '24 14:05 kasugaijin

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.

atbalaji avatar May 31 '24 06:05 atbalaji

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

kasugaijin avatar May 31 '24 14:05 kasugaijin

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.

github-actions[bot] avatar Jul 09 '24 00:07 github-actions[bot]

Automatically unassigned after 7 days of inactivity.

github-actions[bot] avatar Jul 16 '24 00:07 github-actions[bot]

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

wiliajc87 avatar Nov 08 '24 18:11 wiliajc87

@wiliajc87 thank you!

kasugaijin avatar Nov 08 '24 20:11 kasugaijin