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

1703 4 hard delete attachments #1835

Closed
wants to merge 77 commits into from
Closed

Conversation

BCerki
Copy link
Contributor

@BCerki BCerki commented Aug 4, 2023

card: https://app.zenhub.com/workspaces/climate-action-secretariat-60ca4121764d710011481ca2/issues/gh/bcgov/cas-cif/1703

The card is only about hard deleting attachments in the first revision. I think there may be some other cases in which we'd like to hard delete (e.g., deleting an attachment that was uploaded in the same form change) and made a tech debt ticket to investigate: https://app.zenhub.com/workspaces/climate-action-secretariat-60ca4121764d710011481ca2/issues/gh/bcgov/cas-cif/1776

Also, hard deleting attachments as part of discarding a project revision was split into another card: https://app.zenhub.com/workspaces/climate-action-secretariat-60ca4121764d710011481ca2/issues/gh/bcgov/cas-cif/1798

Also also, cypress CI upload and deletion will be another card: https://app.zenhub.com/workspaces/climate-action-secretariat-60ca4121764d710011481ca2/issues/gh/bcgov/cas-cif/1839

@BCerki BCerki force-pushed the 1703-4-hard-delete-attachments branch 9 times, most recently from 7468350 to 1b8a2a2 Compare August 4, 2023 17:45
@BCerki BCerki force-pushed the 1703-4-hard-delete-attachments branch from 56cf511 to 46eb659 Compare August 4, 2023 20:30
@BCerki BCerki marked this pull request as ready for review August 4, 2023 22:05
@BCerki
Copy link
Contributor Author

BCerki commented Aug 4, 2023

@dleard this is the attachments PR, and this is where Gurjeet fixed the middleware: #1838

Copy link
Contributor

@Sepehr-Sobhani Sepehr-Sobhani left a comment

Choose a reason for hiding this comment

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

Awesome! I just have a few leftover things and some curiosity questions :) Once the attachment upload feature is fixed, I would love to manually test this function.

Comment on lines +175 to +178
# - name: Setup tmate session
# uses: mxschmitt/action-tmate@v3
# with:
# detached: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover!

@@ -13,3 +13,5 @@ cypress.env.json
app/cypress.env.json
dev/db/data
**/.DS_Store

gha-creds-*.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to have any files locally related to this rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is leftover from experimenting with CI, will remove


cy.fixture("e2e/mock.pdf").as("mockFile");
cy.get("input[type=file]").selectFile("@mockFile");
cy.wait(30000);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure you have tested this but just as a sanity check, have you tried using any other assertions rather than using wait? or something like this: https://docs.cypress.io/api/commands/selectfile#Custom-fileName-mimeType-and-lastModified


const handleArchiveAttachment = (attachmentId) => {
if (isFirstRevision) {
console.log("in if handlearchiveattachment");
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover!

};

console.log("connectionid", connectionId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover!

attachment: { file, id },
},
} = queryResponse;
console.log("req.body.variables", req.body.variables);
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover!

Comment on lines +57 to +58
// brianna this isn't working since change to the discard mutation--I think this middleware isn't actually transactional. If this doesn't work the next mutation still fires
console.log("attachmentFormChangeResponse", attachmentFormChangeResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover!

Comment on lines +5 to +7
drop function cif.discard_project_attachment_form_change;
create function cif.discard_project_attachment_form_change(form_change_id int)
returns cif.form_change
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious, why not use "create or replace function ..." instead of deleting and creating a new one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the return type is different you can't replace, you have to drop and remake

Copy link
Contributor

Choose a reason for hiding this comment

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

Woooo, good to know 👍

@BCerki
Copy link
Contributor Author

BCerki commented Aug 10, 2023

work finished in #1843

@BCerki BCerki closed this Aug 10, 2023
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 this pull request may close these issues.

3 participants