Attach new files to Moped funding records #1824
Attach new files to Moped funding records #1824chiaberry wants to merge 16 commits intomike/27387_ecapris_file_attachmentsfrom
Conversation
❌ Deploy Preview for atd-moped-main failed. Why did it fail? →
|
…nding-file-attachments
mddilley
left a comment
There was a problem hiding this comment.
Looking great, Chia! Cool to see that the branching between the eCAPRIS and Moped records isn't bad at all imo. 😎 I found a small bug, but I was able to patch, test, and see attachments to both records types. 🙌
I'll come back and wrap the review once you've had a chance to look at the one comment.
frankhereford
left a comment
There was a problem hiding this comment.
When I get to step 4 in the testing instructions, "Now either add another funding row (not imported from eCAPRIS) or edit an existing row so the record is not synced from eCAPRIS.", when I submit that form, I get the following trapped error:
field 'moped_funding_files' not found in type: 'moped_proj_funding_insert_input'
I think the field may be missing from the gridRecord assignment on line 110 of the ProjectFunding helpers.js file.
I think this is a blocker on testing, and I will come back to this PR after dev sync today when I can ask about what you think about these findings.
Thanks for this PR Chia!! I feel rusty reviewing moped, so if I'm just out in left field, lmk and i'll do whatever I am missing. Thanks!
mddilley
left a comment
There was a problem hiding this comment.
I tested this every which way that I could, and I was able to attach and detach files to both eCAPRIS and Moped funding records. ✨ Thanks and this also helped me catch a small bug that I introduced into the local stack activity log that I'll PR shortly. 🚢 🙌 🚢
|
@chiaberry I was reviewing issues today, and I realized that I missed scoping the detail of clearing Moped funding file attachments when the file itself is soft-deleted. So, I created cityofaustin/atd-data-tech#28174 to capture in future work. |
|
I got the same error as @frankhereford |
|
@Charlie-Henry I tried running through the test steps again and couldnt replicate the error. did it happen on a new empty project, or was it on an existing project? |
johnclary
left a comment
There was a problem hiding this comment.
I tested this heavily and it works beautifully! Nice!
| entity_id int4 NOT NULL REFERENCES public.moped_proj_funding(proj_funding_id) ON DELETE RESTRICT ON UPDATE CASCADE, | ||
| file_id int4 NOT NULL REFERENCES public.moped_project_files(project_file_id) ON DELETE RESTRICT ON UPDATE CASCADE, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), |
There was a problem hiding this comment.
i can't remember how the updated_at timestamp is managed. is this missing a trigger?
There was a problem hiding this comment.
i left this out of scope since we were only creating and hard-deleting records, but since I'm rolling the soft deletes into cityofaustin/atd-data-tech#28174 - i'll add this there too. that way we will be able to see when the soft-delete update happened on the attachment tables. 🙏
| : "moped_funding_files"; | ||
| if (!row?.[filesType]) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This awkward accessor problem sorta hints to me that it could be nice to combine the two files arrays upstream (client side, or in a view?) so that you can simply map over them here.
Depending on where this functionality of arbitrary file-entity attachments is heading, it might be worth it now to make a project_files_view or whatever that organizes this data more cleanly for the app to consume it.
There was a problem hiding this comment.
Couple of impressions now that I'm seeing this modal for a second time:
- The modal header has slightly different padding and is missing the close button (
X) vis a vis the funding record edit modal. - I think it could be good to move the submit/cancel buttons to be immediately below the attach file form, to make it clear the attached files are not part of the form/action.
- I wonder if moving the attached files to the top of the modal would make this slightly more intuitive 🤔
There was a problem hiding this comment.
this is confirming a feeling that i've had that we could benefit from a reusable dialog component. this one is baked into the file upload dialog currently, and i felt friction when writing code in here that could be reduced by pulling out that dialog wrapper (along with keeping our dialogs looking similar across the board).
I'm going to aim to implement these suggestions in cityofaustin/atd-data-tech#27484
mateoclarke
left a comment
There was a problem hiding this comment.
Tested locally and looks good.
I was having a similar error that @Charlie-Henry and @frankhereford called out, but running hasura metadata apply seemed to resolve it for me.
| () => rows.find((row) => row.id === fileAttachmentId), | ||
| [rows, fileAttachmentId] | ||
| ); | ||
| const isSyncedFromECapris = fundingRecord.is_synced_from_ecapris; |
There was a problem hiding this comment.
wondering here if fundingRecord could ever return null (maybe if the fileAttachmentId is stale) and could throw an error on line 43. Maybe add a guard like:
const isSyncedFromECapris = fundingRecord?.is_synced_from_ecapris ?? false;
Associated issues
cityofaustin/atd-data-tech#27483
Testing
URL to test:
test locally, use prod data
Steps to test:
This PR builds off of the work in #1820
Open a project (or create a new one)
Navigate to the funding table. Select an eCAPRIS subproject id like
10553.009Like the previous PR, you see funding rows, paperclip icons and Files column.
Now either add another funding row (not imported from eCAPRIS) or edit an existing row so the record is not synced from eCAPRIS.
Click the paperclip to open the attachment dialog.
Toggle the link to file switch and enter a name, type, description and a drive path or link.
You can see the file in the files column of this table, also go to the Files tab and see it there as well.
use your db client to confirm that this record is in the correct table.
The file is still on the Files tab but is no longer on the funding table.
Ship list