Announcement type is removed without removing related files

[jonasraoni]

Describe the bug
After the introduction of foreign keys, the assignment type is removed, but the related images are not.

PRs

• Bugfix stable 3 4 0 10060 fix announcement type removal #10064
• pkp/pkp-lib#10061 Submodule update ##jonasraoni/bugfix-stable-3_4_0-1… ojs#4318

What application are you using?
OJS 3.4

[jonasraoni]

@bozana could you please review this one too? The fixes are on the first 2 commits, the last one is mostly about typing/formatting.

[bozana]

Hi @jonasraoni, here I should wait for you to take a look at nullable that we mentioned in the dev call, or?

[jonasraoni]

Hi! I think you can proceed with the review, most of the updates here are not related to the issue.

Then, I'll create a generic issue to handle the removal of entities that have dependents. The current solution is to block the operation, until the user clears all the dependencies manually... We can offer more options (clear/re-assign the dependencies to another record) to simplify the maintenance.

[bozana]

@jonasraoni, a question before I finish the review:
Isn't it so that the announcement title should always exist? -- I see it is nullable in the json schema, but it is a required field in the form. It seems logical to me that the title needs to be there, no? Also the datePosted?
The same for announcement type name?

[bozana]

Ah, and why do we delete announcements when deleting an announcement type? Shouldn't the type_id just be set to null?
In the Announcement migration we say:
$table->foreign('type_id')->references('type_id')->on('announcement_types')->onDelete('set null');

Thus, I would think we shouldn't do anything else when deleting an announcement type?

[jonasraoni]

Hi! I didn't dig so deep, I just applied some fixes/updates, but I agree with everything.

• Cascading/set null: I've created another issue to check if there are other cases: Inspect cascading rules to avoid unexpected side-effects #10096
  But as the database was already fixed, I'll do the update in this PR 👌
• title/datePosted: Yeah, the nullable validation together with the required sounds odd. There are other cases in the codebase (see the category.json). I'll inspect what the nullable is doing and address the issue here as well.

[bozana]

Thanks @jonasraoni, that all sounds good!

[jonasraoni]

@bozana FYI, the nullable is required. That's how it works:

• required: Forces the user to fill at least the main locale of a multilingual field.
• nullable: Allows the user to leave the remaining locales empty, when it's not present, the user has to fill all locales with something.

[bozana]

@jonasraoni, let me know when another review round can be done... Thanks!!!

[jonasraoni]

@bozana, I think I've addressed all comments, please take a look when you get some free time.

[jonasraoni]

@bozana I almost forgot about this PR, I've updated the getDatePosted() to return null, which was the last comment that I saw.

Regarding the nullable, just to confirm, most of the entities would not fail because we have a lot of non-used code. Very few entities are using the Repository::validate($object), most of them are relying on the Form validation.

[bozana]

Hi @jonasraoni, please go ahead and merge. Let me know if I need to take a look at something when you port it to other branches -- e.g. if you do more changes for the main branch...
Thanks!