-
Notifications
You must be signed in to change notification settings - Fork 35
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
7.x islandora 2046: Allow Islandora Batch to be extended for more fun #105
base: 7.x
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,9 +8,10 @@ | |
/** | ||
* Batch interface. | ||
* | ||
* Implementing classes should subclass some version of FedoraObject, so | ||
* Implementing classes should subclass some version of FedoraObject, so. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize this was there previously, but... this isn't quite a full sentence, here... Really... not sure the intent of this comment... just seems to reinforce the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally. I was more like waiting for someone to complete the sentence! but yeah, can remove |
||
*/ | ||
abstract class IslandoraBatchObject extends IslandoraNewFedoraObject { | ||
|
||
/** | ||
* The initial batch state. | ||
* | ||
|
@@ -71,6 +72,34 @@ abstract class IslandoraBatchObject extends IslandoraNewFedoraObject { | |
*/ | ||
abstract public function addRelationships(); | ||
|
||
/** | ||
* The actual back-end action that is run after the batchProcess. | ||
* | ||
* Classes not overriding this will get objects pushed to the back-end. | ||
* (ingested) | ||
* | ||
* @return AbstractObject|false | ||
* Either a newly ingested Abstract Object or false if failed. | ||
*/ | ||
public function batchRepositoryAction() { | ||
if (!islandora_object_load($this->id)) { | ||
return islandora_add_object($this); | ||
} | ||
else { | ||
return FALSE; | ||
} | ||
} | ||
|
||
/** | ||
* A human readable short description of the repository action. | ||
* | ||
* @return string | ||
* Recomended value is a verb in past tense. | ||
*/ | ||
public function getRepositoryActionDescription() { | ||
return "ingested"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The manner in which this is implemented/used will lead to messages which are not quite appropriately translatable... as is, may lead to incorrect grammar in some languages. ... Would have to allow each implementation to provide their own complete messages (passing the arguments to receive a full response)? Or just the individual template strings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would that mean generating full messages for success, exceptions and errors? Issue i see with that is that adding then afterwards arguments coming from the actual batch would make things quite complicated. What do you propose There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adam-vessey @DiegoPino Perhaps something along the lines of what I suggested earlier ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As indicated... allow implementations to either provide complete messages or template strings. Or a particular implementation approach? Kind of leaning towards the template things... Possibly one of:
... there are a number of ways it could be accomplished. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, will work on the template strings idea, probably 3 ones would suffice. Wonder in that case if the full generation t() et all // - hate having t()in a class...- + argument expansion/replacement should happen an implementing class or should stay on batch itself as it is today? ideas @d-r-p @adam-vessey @bryjbrown ? The simpler the better i would say because the more i add the more i will be open other wormholes .. the implementation would be aware that batch itself only provides the %pid really...and some Drupal exception message... |
||
} | ||
|
||
/** | ||
* Get the ID of this object in the queue. | ||
* | ||
|
@@ -92,6 +121,10 @@ abstract class IslandoraBatchObject extends IslandoraNewFedoraObject { | |
))); | ||
} | ||
} | ||
|
||
} | ||
|
||
/** | ||
* An Islandora Batch Exception Class. | ||
*/ | ||
class IslandoraBatchNoIdException extends Exception {} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -45,7 +45,7 @@ function islandora_batch_schema() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'not null' => FALSE, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'primary key' => array('id'), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'primary key' => array('id', 'sid'), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not... sure this is valid, given all the joins which happen... everything thing that refers to this as a foreign key would be broken, such as the state, resource and message tracking (the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "broken" might be the wrong word... more like... consistent in unexpected ways, when navigating the GUI? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adam-vessey look at In any case Foreign keys will/won't be broken at all and without any extra stuff functionality stays the same with this change. Drupal makes no use of Foreign keys (see them as annotation properties for future work that won't happen anymore). Best example is that our foreign keys are already wrongly defined (see how the gist changes that), we were not using 'columns' key and still nothing ever broke. The way i see this is that this primary key is more an internal optimization to A) disregard the integrity table conflict/error and B) make search in the table faster. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to add to the Foreign key conversation https://interworks.com/blog/jkhalaj/2012/04/12/drupal-7-usage-foreign-keys-schema-api-and-current-default-fk-erd/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At a glance, the gist looks fine (a diff/something highlighting the changes would be more readable), but it's not so much the schema itself I'm concerned with, 'cause yeah, foreign keys are just descriptive as you point out; however, they're nice to keep up-to-date for to facilitate these types of discussions... What I'm more concerned with is more the things like the various state checks:
... The checks on parents as well would likely have to change, since they can no longer be the single columns check... probably safe to constrain to the same set: islandora_batch/includes/ingest.batch.inc Line 293 in c4f31b7
... There would be some views adjustments also in order, to constrain things to items in the given set:
... along with adjusting the menu routes used by views to accept the set:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally. I agree with you @adam-vessey . Those changes are the ones i was speaking about to make this pull not only something that does not break existing functionality and allows other cases, but be fully robust on those future cases. If you agree with the general structure of that gist, i can take that and move forward with changing every place there the assumption is a PID is the only key to rule them all to PID, SET based display and functionality. Will see what my agenda allows me this week, but should be pretty trivial, have already annotated all my findings + yours = feel all based would be covered. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, coming back to this, sorry for the long wait. Will have an update this week. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'indexes' => array( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'parent_index' => array('parent'), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'sid' => array('sid'), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -358,3 +358,11 @@ function islandora_batch_update_7104() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
'not null' => FALSE, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* Make PID and Set ID combined primary Key. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
function islandora_batch_update_7105() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
db_drop_primary_key('islandora_batch_queue'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
db_add_primary_key('islandora_batch_queue', array('id', 'sid')); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
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.
Not sure, but should
ingest
not be replaced by the selected action (if so, perhaps re-wording to'%pid cannot be %action: object not ready.'
could help with the grammar...)?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, totally. Only issue i had there is verb conjugation/my-lack of wording. I don't know how to convert that phrase to "simple past", since "ingested" is what i was using/suggesting. I could, instead of returning a single "action" description, maybe return an array, with present/past verbs. Ideas? Or other way, change wording of other past-dependant sentences to present?
Also, there are many other mentions (like function names) to ingest, i tried to go for a simple solution for now that allowed the use case to avoid too much refactoring. Reason: every-time i have done big-time refactoring everyone ends using my branch and patch, but i don't get merged into core, too much of a commitment!
Totally open to suggestions
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..
'Successfully %action %pid'
which leads to"successfully ingested islandora:monster"
could be reworded as
'We managed to successfully ingest islandora:monster'
? or Islandora managed to, or Islandora Batch, etc..or
"Your Ingest of islandora:monster was successful"
What do you think @d-r-p ? That would a single present tense verb to be used to describe the action itself
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.
Well, I think the more compact the message is, the better. On the other hand, leaving past tenses lying around is somehow awkward (by the way, I guess it would be a good idea to wrap whatever we decide on with
t()
) - let alone present + past tenses. If you do not like my earlier suggestion of reformulating that one message into past tense, how about returningt("Ingest")
and alerting the user with'%action action for %pid succeeded.'
,'%action action for %pid failed.'
, etc... (or similar)?Regarding the naming of functions (and files?), perhaps a little bit of refactoring would be good. I think, otherwise, this might cause confusion if picked up after some time. But feel free (you, or anybody) to disagree...
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, I missed your earlier suggestion! (sorry), yeah, sounds fine to me and i will go for that, thanks. t() and watchdog don't play well... since we are kinda accumulating the messages. I remember having quite some issues with that in the past so will stay away from that now. Also, i always forget our classes are really not generic php classes, they are used inside the weirdness of drupal, so t() can be used...
About naming of functions and files. Refactor can bring fear and riots as said before. Will wait a bit and if someone else feels it is needed right now can do of course. I would need to deprecate some functions and maybe too much. Lets discuss this. This was meant to actually be eventually merged!