-
Notifications
You must be signed in to change notification settings - Fork 911
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.
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
pkp/pkp-lib#2072 Versioning for published articles #1277
Conversation
8fd2eeb
to
589ac39
Compare
8152d0d
to
4b59bfb
Compare
classes/article/ArticleDAO.inc.php
Outdated
@@ -182,11 +183,14 @@ function updateObject($article) { | |||
); | |||
|
|||
$this->updateLocaleFields($article); | |||
$contextId = Request::getContext()->getId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is always better to try not to use Request statically -- whenever possible. So maybe to get the context id from the article object here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better still, the Request object shouldn't be used in the DAOs and DataObjects at all -- that's kind of mixing the Controller with the Model. Isn't the context_id
(journal_id
) available as a column here already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can read the contextId from the article: $article->getContextId()
ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you do not need that parameter contextId at all -- if you see my other comments... Also, Alec proposed to have another DB column in the submissions table with the current/latest version, I believe.... So maybe to first go roughly over all comments and then start with changes... ?
classes/article/ArticleDAO.inc.php
Outdated
for ($i=0, $count=count($authors); $i < $count; $i++) { | ||
if ($authors[$i]->getId() > 0) { | ||
$authors[$i]->setVersion($version); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to set the version here? -- because we search after the authors by the version (getAuthors (false, $version)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, seems to be redundant. Removed it. 🐛
WHERE g.submission_id = ? | ||
AND nsf.file_id IS NULL | ||
' . ($contextId?' AND s.context_id = ? ':'') . ' | ||
' . ($submissionRevision?' AND sfg.version = ? ':'') . ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the . '
in the line before this line, as well as '
in this line are not necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks. Its gone now.
) | ||
); | ||
$galley->setId($this->getInsertId()); | ||
$this->updateLocaleFields($galley); | ||
$this->addFile($galley->getId(), $galley->getFileId(), $galley->getCurrentVersionId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should here getCurrentVersionId
be used or the current galley (that we are inserting) version number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you can only add galleys to the most recent version it is always the recent version id (Old versions can not be edited). Maybe this should be made clearer a the comments of the function ...
|
||
HookRegistry::call('ArticleGalleyDAO::insertNewGalley', array(&$galley, $galley->getId())); | ||
|
||
return $galley->getId(); | ||
} | ||
|
||
/** | ||
* Connect galley version with file in submission_galley_files | ||
* @param $galleyId ArticleGalleyId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
(galley_id, file_id, version) | ||
VALUES | ||
(?, ?, ?) | ||
ON DUPLICATE KEY UPDATE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that not all DB engines act in the same way on this command, e.g. see MySql documentation: "The effects are not identical for an InnoDB table where a is an auto-increment column. With an auto-increment column, an INSERT statement increases the auto-increment value but UPDATE does not.". Also, PostgreSQL seem to have different command/syntax. Thus it wold be better to differentiate it into simple standard SQL statements insert and update, I believe...
$this->update('DELETE FROM submission_galley_settings WHERE galley_id = ?', array((int) $galleyId)); | ||
$this->update(' | ||
DELETE FROM submission_galley_settings | ||
WHERE galley_id = ? AND version = ?', array((int) $galleyId, $submissionRevision)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(int) cast for submission revision would be good too
' . ($pubIdType != null?' LEFT JOIN submission_galley_settings gs ON (g.galley_id = gs.galley_id)':'') | ||
. ($title != null?' LEFT JOIN submission_settings sst ON (s.submission_id = sst.submission_id)':'') | ||
. ($author != null?' LEFT JOIN authors au ON (s.submission_id = au.submission_id)':'') | ||
. ($pubIdSettingName != null?' LEFT JOIN submission_galley_settings gss ON (g.galley_id = gss.galley_id AND gss.setting_name = ?)':'') .' | ||
WHERE | ||
i.published = 1 AND s.context_id = ? | ||
AND ss.setting_name = "datePublished" AND ss.submission_revision = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always when possible single quotes '
should be used
LEFT JOIN issues i ON ps.issue_id = i.issue_id | ||
' . $this->getFetchJoins() . ' | ||
WHERE i.published = 1 | ||
' . ($journalId?'AND s.context_id = ?':'') . ' | ||
AND s.status <> ' . STATUS_DECLINED . ' | ||
ORDER BY ps.date_published '. ($reverse?'DESC':'ASC'), | ||
AND ss.setting_name = "datePublished" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single quotes...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case using single quotes would break the sql statement because datePublished needs to be a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With escape \'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lilients, standard SQL syntax is for strings to be quoted with '
-- MySQL is OK with "
as well, but I think PostgreSQL isn't.
@@ -273,15 +279,18 @@ function getPublishedArticlesBySectionId($sectionId, $issueId) { | |||
*/ | |||
function getPublishedArticleById($publishedArticleId) { | |||
$result = $this->retrieve( | |||
'SELECT * FROM published_submissions WHERE published_submission_id = ?', (int) $publishedArticleId | |||
'SELECT ps.*, ss.setting_value FROM published_submissions ps | |||
JOIN submissions_settings ss ON (ss.submission_id = ps.submission_id) WHERE ss.setting_name = "datePublished" AND |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe to put the WHERE clause in the next line?
And single quotes...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the escape, I think like this: \'
@@ -221,6 +224,9 @@ function getPublishedArticlesInSections($issueId, $useCache = false) { | |||
$publishedArticles[$currSectionId]['title'] = $publishedArticle->getSectionTitle(); | |||
} | |||
} | |||
// get metadata of the current version | |||
$publishedArticle = $this->getPublishedArticleByArticleId($publishedArticle->getId(), null, false, $publishedArticle->getCurrentVersionId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... It looks like everywhere else in this class the submission revision 1 is used to get the published articles, using _fromRow without the parameters. For example in getPublishedArticlesByJournalId. Why then here only the current article version?
It seems that the concept is to get the first and then later to get the current version, or? If so maybe to do this here as well?
Maybe to take a look how those functions are used elsewhere? -- e.g. in the generic plugin WebFeedGatewayPlugin both functions getPublishedArticlesByJournalId or getPublishedArticlesInSections are used, but they would return different article versions, I suppose?
It would be even better if we could always immediately provide the latest article versions in this class, or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed _fromRow()
to get the latestRevisionId
if submissionRevision==null
|
||
$submission = $submissionDao->getById($submissionId); | ||
$submission->setDatePublished($datePublished); | ||
$submissionDao->updateLocaleFields($submission); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just to use the SQL insert statement instead of getting the submission objects and update them -- the upgrade scripts work with SQL statements, so it would be OK here to do so, I suppose... Maybe just only one SQL statement for everything (for the whole function) would also work? -- In that case we could put it in an XML file in the upgrade folder...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need the information before the DB XML schemas are executed and thus the columns removed, you can create a temporary table in a preupdate script, s. for example commentsToEditor migration and dbscripts/xml/upgrade/3.0.0_preupdate_commentsToEditor.xml.
if(isset($fileId) && $fileId != 0){ | ||
$galley = $galleyDao->getById($galleyId); | ||
$galley->setFileId($fileId); | ||
$galleyDao->updateObject($galley); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the same as above -- Maybe only one insert SQL statement would do the job...
issues i, | ||
submission_search_objects o NATURAL JOIN ' . $sqlFrom . ' | ||
WHERE | ||
s.submission_id = o.submission_id AND | ||
s.status = ' . STATUS_PUBLISHED . ' AND | ||
ps.submission_id = s.submission_id AND | ||
ss.submission_id = s.submission_id AND | ||
ss.setting_name = "datePublished" AND |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single quotes...
dbscripts/xml/upgrade.xml
Outdated
@@ -131,6 +133,12 @@ | |||
<note file="docs/release-notes/README-3.0.1" /> | |||
</upgrade> | |||
|
|||
<upgrade minversion="3.0.0.0" maxversion="3.1.0.0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the maxversion is 3.0.9.9 (before 3.1)
dbscripts/xml/upgrade.xml
Outdated
@@ -116,6 +116,8 @@ | |||
<upgrade minversion="3.0.0.0" maxversion="3.0.0.9"> | |||
<code function="setFileUploader" /> | |||
<code function="setFileName" /> | |||
<code function="migrateDatesPublished" /> | |||
<data file="dbscripts/xml/upgrade/3.0.0_versioning.xml" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can name it 3.1.0_...
dbscripts/xml/upgrade.xml
Outdated
<upgrade minversion="3.0.0.0" maxversion="3.1.0.0"> | ||
<code function="migrateDatesPublished" /> | ||
<code function="migrateGalleyFiles" /> | ||
<data file="dbscripts/xml/upgrade/3.0.0_versioning.xml" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here 3.1.0_...
* 3.0.0_versioning.xml | ||
* | ||
* Copyright (c) 2014-2016 Simon Fraser University Library | ||
* Copyright (c) 2003-2016 John Willinsky |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
year 2017 :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I've recently changed
Copyright (c) xxxx-yyyy Simon Fraser University Library
...to...
Copyright (c) xxxx-yyyy Simon Fraser University
...as SFU is the legal entity, not the SFU library.
<!-- authors --> | ||
<query>UPDATE authors SET version = 1 WHERE version IS NULL OR version = 0</query> | ||
<query>UPDATE author_settings SET version = 1 WHERE version IS NULL OR version = 0</query> | ||
<query>ALTER TABLE authors DROP PRIMARY KEY, ADD PRIMARY KEY (author_id, version)</query> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this not happen automatically, when the keys are defined in the DB schema xml?
<query>ALTER TABLE authors DROP PRIMARY KEY, ADD PRIMARY KEY (author_id, version)</query> | ||
<query>UPDATE submission_galley_settings SET version = 1 WHERE version IS NULL OR version = 0</query> | ||
<query>UPDATE submission_galley_files SET version = 1 WHERE version IS NULL OR version = 0</query> | ||
<query>ALTER TABLE submission_galleys DROP COLUMN file_id</query> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also happen automatically, when the appropriate xml DB schema is run...
And the next two lines as well...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S. also comment above how to solve the issue when you need information before the DB XML schemas are executed, so that you would not need this code...
pages/article/ArticleHandler.inc.php
Outdated
@@ -17,18 +17,30 @@ | |||
import('classes.handler.Handler'); | |||
|
|||
class ArticleHandler extends Handler { | |||
/** journal associated with the request **/ | |||
/** @var journal object: journal associated with the request **/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…ion history changed
See pkp/pkp-lib#2072.