LF-4701 Call DELETE endpoint when clicking disconnect from Farm Addon #3696

kathyavini · 2025-02-20T23:15:27Z

Description

This PR connects the delete endpoint from @Duncan-Brain's PR #3692 to the frontend of app, wrapping up the disconnect user story and the whole FarmAddon arc (🎉!)

There weren't any snackbars on this flow in Figma so I didn't include them, but have been wondering about that. I think it would be okay to exclude because there is no 'success' snackbar for adding an addon (you are routed to the sensor list instead) and with the disconnect confirmation modal it would be a bit of a 'noisy' screen with two popups in a row. But it also kind of depends on how much it is supposed to feel like deleting another entity (e.g. task, animal, transaction) which always snackbar 🤔 I think I'm okay with the snackbar-less UX here, but what do you guys think? Should we check with Loïc?

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

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?

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

… returned id

Duncan-Brain · 2025-02-21T14:37:44Z

If we are moving away from snackbars it would be nice to know the specific situations or if all app is moving away. I would otherwise assume Loic just forgot to add it in to the designs and we follow our normal pattern (even for success).

SayakaOno

I probably wouldn't have noticed that snackbars aren't implemented if you hadn't mentioned it, but I feel like having feedback for the user's actions is nice 🤔

packages/webapp/src/store/api/apiSlice.ts

…nt with our current preferences

kathyavini · 2025-02-25T23:18:14Z

I have made the following changes to this branch:

added snackbars
fixed the delete mutation return type (thank you @SayakaOno!)
alphabetised some keys by running pnpm i18n now that we are able to (again, thank you Sayaka!!)

And then in looking at more closely at fail conditions around farmAddon:

conditionally invalidated tags, see https://stackoverflow.com/a/75952409 -- actually I didn't know the default behaviour was to invalidate even if the mutation errors; that doesn't seem like what we want
altered the farmAddon GET to return an empty array!! Actually the 404 had been bugging me the whole time since we had just decided on the other pattern (200 + empty array) in this recent conversation here. Using the success/error distinction as had been done originally here meant API error = farm addon disappears from frontend. It seems more reasonable that the API failing should have no effect on the visibility of the addon -- only a true "no addons exist" response should clear the existing cache.

Thank you for your comments and if you could kindly please re-review 🙏

SayakaOno

Looks great!
Nice find on the invalidatesTags, and returning [] instead of an error feels right ☺️

One thing I noticed is that the button's focused(?) style affects the text at smaller browser sizes. I think we could adjust it when we have time!

Screen.Recording.2025-02-26.at.1.05.23.PM.mov

Duncan-Brain

Nice, I also asked Anto last week if I could change the 404 in the PR where she scoped it! https://lite-farm.atlassian.net/browse/LF-4694

I added one comment for you to consider -- also feel free to update the delete return status to 204 if you want to to make the snackbar and store work properly on disconnect. I have added it in another PR but QA will probably flag it here first.

Duncan-Brain · 2025-02-26T23:17:03Z

packages/webapp/src/containers/Profile/Farm/Addons/index.tsx

 import { PARTNERS } from '../../../AddSensors/constants';

 const FarmAddons = () => {
-  const { isSuccess: hasEsciConnection, data: esciDataArray } = useGetFarmAddonQuery(
+  const { data: esciDataArray = [] } = useGetFarmAddonQuery(


Instead of providing a default value should we be using the isLoading to determine hasActiveConnection (and therefore if we should render) ?

I'm not sure if I follow because isLoading can be false (loaded) and hasActiveConnection also false (because there are no active farm addons). However there IS a little blip not here but in <Partners /> that I should probably have fixed with isLoading!

I would like to clean up that blip along with @SayakaOno's button issue too -- I hate those little visual things 🤣 So I'll make a follow up PR to prevent scope creep on this one.

also feel free to update the delete return status to 204 if you want to to make the snackbar and store work properly on disconnect. I have added it in another PR but QA will probably flag it here first.

In what sense are you finding the snackbars to still not work properly such that QA would flag? I don't think RTK Query does anything different with a 200 vs 204 and they are working properly for me 🤔

In the sense if it rendered using the [] it could be incorrect -- possible blip somewhere ;) Sounds good to do in other PR

In what sense are you finding the snackbars to still not work properly such that QA would flag? I don't think RTK Query does anything different with a 200 vs 204 and they are working properly for me 🤔

Do you not receive a error snackbar when disconnecting? Backend works correctly but my error snackbar shows because it cant parse the "O" in "OK" lol from the 200 response -- I thought that was what Sayaka was seeing also in her comment. I have to relog to see updates on frontend. The 204 fixes it.

Alright, I've changed the response code but let us pocket this as a fun mystery to someday solve.

@antsgar I am looping you in too because this is something fun and wacky. With no differences that we can detect between our @reduxjs/toolkit versions (or anything else that seemed potentially relevant), Duncan and Sayaka get an error on the .unwrap() of a .sendStatus(200) from the controller -- and for me it works just fine.

Any of these solve the issue: .sendStatus(204), .status(200).send() or responseHandler: "text" in apiSlice (from this StackOverflow) but the question is why the inconsistency between our machines 😄 Same branch, same packages.

(Error screenshot in thread above here)

kathyavini · 2025-02-27T00:08:17Z

Nice, I also asked Anto last week if I could change the 404 in the PR where she scoped it! https://lite-farm.atlassian.net/browse/LF-4694

❤️ I think dev team is now completely united in our dislike of the 404 pattern 😂

kathyavini added 2 commits February 20, 2025 14:52

LF-4687 Add deletion endpoint to RTK Query and update type with newly…

6b7ec65

… returned id

LF-4687 Hook up disconnect to FarmAddon component

5b69d06

kathyavini added the enhancement New feature or request label Feb 20, 2025

kathyavini self-assigned this Feb 20, 2025

kathyavini requested review from a team as code owners February 20, 2025 23:15

kathyavini requested review from Duncan-Brain and SayakaOno and removed request for a team February 20, 2025 23:15

SayakaOno reviewed Feb 21, 2025

View reviewed changes

packages/webapp/src/store/api/apiSlice.ts Outdated Show resolved Hide resolved

kathyavini added 5 commits February 25, 2025 13:57

LF-4701 Fix return type of FarmAddon delete mutation

5e844ac

LF-4701 Only invalidate tags if mutation is successful

bc4c6aa

LF-4701 Add snackbars to disconnect action

222e0b1

LF-4701 Commit results of running i18n in webapp (fix ordering of keys)

27e99bb

LF-4701 Return empty array instead of 404 from GET request in alignme…

4049c54

…nt with our current preferences

kathyavini requested a review from SayakaOno February 25, 2025 23:18

SayakaOno previously approved these changes Feb 26, 2025

View reviewed changes

Duncan-Brain previously approved these changes Feb 26, 2025

View reviewed changes

LF-4701 Change delete sendStatus to 204

93611c5

kathyavini dismissed stale reviews from Duncan-Brain and SayakaOno via 93611c5 February 27, 2025 17:26

SayakaOno approved these changes Feb 27, 2025

View reviewed changes

kathyavini requested a review from Duncan-Brain February 27, 2025 18:07

Duncan-Brain approved these changes Feb 27, 2025

View reviewed changes

Duncan-Brain added this pull request to the merge queue Feb 27, 2025

Merged via the queue into integration with commit ff67148 Feb 27, 2025
4 of 5 checks passed

kathyavini mentioned this pull request Feb 27, 2025

Farm Addon & Partner visual adjustments #3700

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LF-4701 Call DELETE endpoint when clicking disconnect from Farm Addon #3696

LF-4701 Call DELETE endpoint when clicking disconnect from Farm Addon #3696

kathyavini commented Feb 20, 2025 •

edited

Loading

Duncan-Brain commented Feb 21, 2025 •

edited

Loading

SayakaOno left a comment

kathyavini commented Feb 25, 2025

SayakaOno left a comment

Duncan-Brain left a comment

Duncan-Brain Feb 26, 2025

kathyavini Feb 27, 2025

Duncan-Brain Feb 27, 2025 •

edited

Loading

kathyavini Feb 27, 2025 •

edited

Loading

kathyavini commented Feb 27, 2025

LF-4701 Call DELETE endpoint when clicking disconnect from Farm Addon #3696

LF-4701 Call DELETE endpoint when clicking disconnect from Farm Addon #3696

Conversation

kathyavini commented Feb 20, 2025 • edited Loading

Duncan-Brain commented Feb 21, 2025 • edited Loading

SayakaOno left a comment

Choose a reason for hiding this comment

kathyavini commented Feb 25, 2025

SayakaOno left a comment

Choose a reason for hiding this comment

Duncan-Brain left a comment

Choose a reason for hiding this comment

Duncan-Brain Feb 26, 2025

Choose a reason for hiding this comment

kathyavini Feb 27, 2025

Choose a reason for hiding this comment

Duncan-Brain Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

kathyavini Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

kathyavini commented Feb 27, 2025

kathyavini commented Feb 20, 2025 •

edited

Loading

Duncan-Brain commented Feb 21, 2025 •

edited

Loading

Duncan-Brain Feb 27, 2025 •

edited

Loading

kathyavini Feb 27, 2025 •

edited

Loading