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

LF-4531: Set up new task abandonment reason for tasks with removed animals #3533

Open
wants to merge 4 commits into
base: integration
Choose a base branch
from

Conversation

SayakaOno
Copy link
Collaborator

@SayakaOno SayakaOno commented Nov 18, 2024

Description

This PR adds support for the new abandonment reason when all animals are removed from a task. The key will be added as part of delete/remove animals/batches APIs.

  • Add a migration to add constraint to validate the abandonment_reason in the task table. (This wasn't necessary, but I added it to avoid adding a wrong key. Wrong keys don't exist in the prod or beta DB, so it should be safe to add this constraint.)
  • Add a translation key for the new reason
  • Add the new reason to TaskModel
  • Add the checkAbandonTask middleware not to allow the new key to be added from clients
  • Add a test

This task was simpler than expected since the keys are hard-coded in the webapp, and the read-only page and task creation page don't share the same code.

  • Creation

    <Controller
    control={control}
    name={REASON_FOR_ABANDONMENT}
    render={({ field }) => (
    <ReactSelect
    options={abandonmentReasonOptions}
    label={t('TASK.ABANDON.REASON_FOR_ABANDONMENT')}
    required={true}
    style={{ marginBottom: '24px' }}
    {...field}
    placeholder={t(`common:SELECT`)}
    />
    )}
    rules={{ required: true }}
    />

  • Read-only

{isAbandoned && (
<div>
<Semibold style={{ marginBottom: '24px' }}>{t('TASK.ABANDONMENT_DETAILS')}</Semibold>
<ReactSelect
label={t('TASK.ABANDON.REASON_FOR_ABANDONMENT')}
required={true}
style={{ marginBottom: '24px' }}
isDisabled={true}
value={{
label: t(`TASK.ABANDON.REASON.${task.abandonment_reason}`),
value: task.abandonment_reason,
}}
/>
{isOtherReason && (
<InputAutoSize
style={{ marginTop: '40px', marginBottom: '40px' }}
label={t('TASK.ABANDON.EXPLANATION')}
value={task.other_abandonment_reason}
disabled
/>
)}

Jira link: https://lite-farm.atlassian.net/browse/LF-4531

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Replaced abandoned task's abandonment_reason with NO_ANIMALS in the DB and checked the read-only page in the webapp.

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain)

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

@SayakaOno SayakaOno self-assigned this Nov 18, 2024
@SayakaOno SayakaOno added the enhancement New feature or request label Nov 18, 2024
@SayakaOno SayakaOno force-pushed the LF-4531/Set_up_new_task_abandonment_reason_for_tasks_with_removed_animals branch from b04061c to 7f797f2 Compare November 21, 2024 18:40
@SayakaOno SayakaOno changed the title [WIP] LF-4531: Set up new task abandonment reason for tasks with removed animals LF-4531: Set up new task abandonment reason for tasks with removed animals Nov 21, 2024
@SayakaOno SayakaOno marked this pull request as ready for review November 21, 2024 18:52
@SayakaOno SayakaOno requested review from a team as code owners November 21, 2024 18:52
@SayakaOno SayakaOno requested review from antsgar and kathyavini and removed request for a team November 21, 2024 18:52
@SayakaOno SayakaOno marked this pull request as draft November 21, 2024 21:02
* update middleware to avoid the reason to be added from the client
* add test
@SayakaOno SayakaOno marked this pull request as ready for review November 21, 2024 21:26
Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry could you elaborate on why you wanted to add the database restriction at this particular stage? I understand it's definitely ideal to restrict at multiple levels, but I'm thinking about that "Crop management plan abandoned" translation ticket coming up, which I think will also require creating a new key, and so that migration would end up deleting and re-creating this check too, then, correct?

Only to restrict the number of database migrations, I wonder if continuing to let the model validation enforce for now would be okay? Or is there a particular danger the model isn't catching?

@antsgar
Copy link
Collaborator

antsgar commented Nov 23, 2024

Similarly to how we do this now for other entities, I think the best way to solve the problem of disallowing wrong keys at the database level would be to have an enum table for the abandonment keys, and have the reason column have a foreign key to that table. Would be similar to what's happening now with animal movement purposes, or animal uses. I'm not sure if that migration would be trickier though due to us already having existing data, so we don't need to do this now! But just proposing that as a longer term solution

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

Successfully merging this pull request may close these issues.

3 participants