Announcement type is removed without removing related files about pkp-lib HOT 12 OPEN

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.

from pkp-lib.

@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.

from pkp-lib.

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

from pkp-lib.

@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.

from pkp-lib.

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

from pkp-lib.

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

from pkp-lib.

@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?

from pkp-lib.

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?

from pkp-lib.

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: #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.

from pkp-lib.

Thanks @jonasraoni, that all sounds good!

from pkp-lib.

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

from pkp-lib.

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!

from pkp-lib.