Skip to content

Update to purge functionality (D832) - #607

Open
droberts-ctrlo wants to merge 12 commits into
ctrlo:devfrom
droberts-ctrlo:file-purge-fix
Open

Update to purge functionality (D832)#607
droberts-ctrlo wants to merge 12 commits into
ctrlo:devfrom
droberts-ctrlo:file-purge-fix

Conversation

@droberts-ctrlo

Copy link
Copy Markdown
Contributor

Specific datum changes are present, as not every datum is purgeable. The system will now mark the record as purged and disallow editing, putting the record into an archived state.
Further discussion may be required as to whether to mark the record as archived within the table as it is displayed as well as current (new) functionality to show the record as archived once the record is opened.

droberts-ctrlo and others added 2 commits March 18, 2026 10:35
Specific datum changes are present as not every datum is purgeable.
The system will now mark the record as purged and disallow editing,
putting the record into an archived state.
Further discussion may be required as to whether to mark the record as
archived within the table as it is displayed as well as current (new)
functionality to show the record as archived once the record is opened.
@droberts-ctrlo droberts-ctrlo changed the title Update to purge functionality Update to purge functionality (D832) Mar 18, 2026

@pwlodarski-ctrlo pwlodarski-ctrlo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall looks good - just one or two clarity comments.

Comment thread lib/GADS/Datum/File.pm
Comment thread lib/GADS/Role/Presentation/Record.pm Outdated

@pwlodarski-ctrlo pwlodarski-ctrlo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR reviewed - appears fine for me.

@pwlodarski-ctrlo pwlodarski-ctrlo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR re-reviewed - minor comments added for consideration.

Comment thread bin/update-cached.pl Outdated
use Dancer2;
use Dancer2::Plugin::DBIC;
use Dancer2::Plugin::LogReport mode => 'VERBOSE';
use Dancer2::Plugin::LogReport mode => 'DEBUG';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we want to this be in DEBUG?

Comment thread lib/GADS/Datum/File.pm Outdated
my $old_content = $old_value ? $old_value->content : undef;
my $old_name = $old_value ? $old_value->name : undef;
if (defined $old_content && defined $old_name) {
if(my $fl = $self->schema->resultset('Fileval')->search({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just for continuity of formatting across the file, would it be worth adding a space after the if ?

Comment thread lib/GADS/Datum/File.pm
{ my $self = shift;
my $return = $self->for_table_template;
$return->{values} = $self->files;
$return->{values} = $self->is_purged ? ["[purged]"] : $self->files;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does Datum::File inherit the is_purged method from a parent object? I see that the local is_purged sub has been deleted.

Comment thread lib/GADS/Role/Purgable.pm Outdated
use strict;
use warnings;

use Log::Report;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is Log::Report used in this file?

@pwlodarski-ctrlo pwlodarski-ctrlo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR re-re-reviewed - appears fine to me!

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.

2 participants