Comments (12)
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, thenullable
validation together with therequired
sounds odd. There are other cases in the codebase (see thecategory.json
). I'll inspect what thenullable
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.
Related Issues (20)
- Convert Queries to an Eloquent model
- RemoveUnvalidatedExpiredUsers never selects any users HOT 1
- Dates Not Displayed in "Invitation To Review" Submission Emails
- OMP does not allow submissions if a press has no active series HOT 5
- Editorial Activity Documentation Bug
- Remove editorialTeam.tpl from theme plugins
- Consider new ORCID implementation in Editorial Masthead and Editorial History page
- Ampersand on publication title breaks the Crossref XML export HOT 1
- [ojs-3.4.0-5, php 8.3.8.r2] Fatal ERROR 500, blank page on article page preview as well as published pages HOT 2
- Article PDF viewer does not use rich-formatted titles
- Locale with variants prevent submission wizard to load HOT 3
- Hide or remove provider's secrets in the administration "system info" page HOT 8
- Outstanding editorial task automatic email content going wrong HOT 5
- Button Orders in OJS
- Side Modal / Pop-up Closing behaviour
- Update StageAssignment model to use eloquence HasCamelCasing trait HOT 2
- Globe icon shows wrong color when more languages are completed than available
- Notification Actions Causes Error
- TypeError: Cannot read properties of null (reading 'match') when adding keywords
- [ops-3.4.0-5, php 8.3.8.r2] the latest preprint of the same day not coming on top HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from pkp-lib.