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.

Sign up for GitHub

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

Jump to bottom

[Issue Tracker] deleting attachment leaves no record of the attachment history #8490

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

[Issue Tracker] deleting attachment leaves no record of the attachment history #8490

wants to merge 21 commits into from

Conversation

GeorgeMurad
Copy link
Contributor

  • Adding new feature to the Issue Tracker module for the Attachment History, when deleting the log on the 'Attachment History' should appear.
  • Adding new line called Date of deletion to the Attachment History when got deleted. It will show the user when did they delete the Upload file(Attachment).

Testing instructions (if applicable):

  1. Create new issue under the issue tracker.
  2. Add attachment.
  3. Upload attachment.
  4. Delete attachment when appears.
  5. The Attachment History should appear.

Related To: #7882

@GeorgeMurad GeorgeMurad changed the base branch from main to 24.1-release April 3, 2023 16:53
@GeorgeMurad
Update attachmentprovisioner.class.inc
8e8417c 
Fix the indentation for mime_type and file_size
@GeorgeMurad
fix_indentation
821de89 
Fixing the indentation for file_size and date_deleted on line 164 and 165
@GeorgeMurad GeorgeMurad changed the title 2023 04 03 deleting attachment leaves no record of the attachment [Issue Tracker] deleting attachment leaves no record of the attachment history Apr 3, 2023
laemtl
laemtl reviewed Apr 4, 2023
View reviewed changes
@GeorgeMurad
Update 0000-00-00-schema.sql
43f30d8 
Remove the semi-column from the `date_deleted`
@GeorgeMurad
Update attachmentsList.js
f1bf77f 
fixing Indentation
@GeorgeMurad
Update 0000-00-00-schema.sql
08afc4b 
setting the `date_deleted` to default null instead of timestamp()
@xlecours xlecours added the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Apr 11, 2023
@laemtl laemtl removed the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Apr 11, 2023
@laemtl
Copy link
Contributor

laemtl commented Apr 11, 2023 •
edited
Loading

@GeorgeMurad Following our roadmap meeting discussion today, can you automatically create a comment when a file is deleted instead? That will avoid changing the SQL schema which is a better option.

@ridz1208 ridz1208 added the State: Needs work PR awaiting additional work by the author to proceed label Apr 25, 2023
@ridz1208
Copy link
Collaborator

@driusan is this a critical bugfix? its going to 24

@CamilleBeau
Copy link
Contributor

@GeorgeMurad Can you please resolve conflicts in this one?

@ridz1208 ridz1208 added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jun 14, 2024
@kongtiaowang kongtiaowang changed the base branch from 24.1-release to main July 15, 2024 15:20
@GeorgeMurad GeorgeMurad reopened this Sep 23, 2024
@GeorgeMurad GeorgeMurad force-pushed the 2023-04-03_deleting_attachment_leaves_no_record_of_the_attachment branch from 08afc4b to 8d48b07 Compare September 23, 2024 18:54
root and others added 6 commits September 23, 2024 15:17
Rebase new changes
f5946cc
@GeorgeMurad
Merge pull request #1 from GeorgeMurad/GeorgeMurad-2023-04-03_deletin…
8ef1bb0 
…g_attachment_leaves_no_record_of_the_attachment

George murad 2023 04 03 deleting attachment leaves no record of the attachment
@GeorgeMurad
Update uploadhelper.class.inc
0f20575
@GeorgeMurad
Update 2023-04-03_Add_Column_to_issues_attachments_table.sql
36aa7e7
@GeorgeMurad
Update attachmentsList.js
42e4139 
Adding the `date_deleted` and removing the deleted column.
@GeorgeMurad
Update 2023-04-03_Add_Column_to_issues_attachments_table.sql
ce6322d
@GeorgeMurad GeorgeMurad requested a review from laemtl October 18, 2024 14:37
@GeorgeMurad GeorgeMurad added Category: Feature PR or issue that aims to introduce a new feature and removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Needs work PR awaiting additional work by the author to proceed labels Oct 18, 2024
@driusan driusan added this to the 27.0.0 milestone Feb 4, 2025
@regisoc regisoc assigned racostas and unassigned GeorgeMurad Feb 17, 2025
@racostas racostas self-requested a review February 20, 2025 15:12
racostas
racostas requested changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@racostas racostas left a comment

Choose a reason for hiding this comment

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

Only two small comments:

  • I think we usually don't send the patch to SQL/Release_patches , I think to remember this will be done when creating the release script for updating LORIS. @driusan could tell more about this than myself.
  • When I was testing this PR in main I got a 500 error that was not caused by the PR but that maybe we should solve as part of it. The issue was a type mismatch for the following variables in the data model attachmentdto.class.inc:
    • $ID, $issueID and $file_size have to be declared int in place of string.
      image

@GeorgeMurad
Copy link
Contributor Author

@racostas I agree with updating the variable types to INT. And how about the SQL/Release_patches?

@racostas
Copy link
Contributor

@racostas I agree with updating the variable types to INT. And how about the SQL/Release_patches?

I think the Patch inside SQL/Release_patches is not necessary because this folder only contains "release patches" that are made from the ones that were already included in SQL/New_patches/ and SQL/Cleanup_patches, @driusan can you confirm this ?? I'm not 100% sure. Thanks.

@racostas racostas requested a review from driusan February 20, 2025 17:44
@driusan
Copy link
Collaborator

driusan commented Feb 20, 2025

Yes. Patches go into New_patches. @ridz1208 or @kongtiaowang usually merge them into a release patch before the release.

@GeorgeMurad
Copy link
Contributor Author

OK, great. I will remove it from the release patches!

@GeorgeMurad GeorgeMurad requested a review from racostas February 20, 2025 18:26
@racostas
Copy link
Contributor

@GeorgeMurad , some test failing: mostly indentation
image

@racostas racostas added the Needs formatting PR requires formatting fixes to meet our coding standards (PHPCS, phan, etc.) label Feb 20, 2025
@GeorgeMurad
Copy link
Contributor Author

@racostas I am not sure what is happening here. I fixed all the indentation automatically using phcbf.

@racostas
Copy link
Contributor

@GeorgeMurad, try to fix the conflicts. Maybe a rebase is need since it seem the latest activity in this PR (before this week) was a while ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CamilleBeau CamilleBeau

@laemtl laemtl

@driusan driusan

@racostas racostas

Requested changes must be addressed to merge this pull request.

Assignees

@racostas racostas

Labels
Category: Feature PR or issue that aims to introduce a new feature Needs formatting PR requires formatting fixes to meet our coding standards (PHPCS, phan, etc.)
Projects
LORIS RoadMap
Status: PR sent
Milestone
27.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@GeorgeMurad @laemtl @ridz1208 @CamilleBeau @racostas @driusan @xlecours