Islandora-Labs / islandora_solution_pack_oralhistories

Adds all required Fedora objects to allow users to ingest and retrieve Oral Histories (video/audio) files through the Islandora interface
GNU General Public License v3.0
13 stars 23 forks source link

Fixes last cue not appearing in the edit form. #139

Closed MarcusBarnes closed 5 years ago

MarcusBarnes commented 5 years ago

What does this Pull Request do?

The last cue will now appear in the edit form when editing the transcript XML.

How should this be tested?

Create a new Oral History object with transcript XML. The correct number of cues should appear in the edit transcript form.

Additional Notes:

Those affected by this issue https://github.com/Islandora-Labs/islandora_solution_pack_oralhistories/issues/134 may have to manually fix their transcript XML.

Thanks to @Natkeeran for catching that the final cue was not appearing after some recent changes.

Interested parties

@BrandonMorr @Natkeeran

MarcusBarnes commented 5 years ago

Note that the Travis build is failing because of issue https://github.com/Islandora-Labs/islandora_solution_pack_oralhistories/issues/136. The changes in this pull-request are minor.

BrandonMorr commented 5 years ago

@MarcusBarnes, apologies for not looking at this sooner. I tested out the changes with this example transcript and it appears this PR reintroduces a bug where an extra empty cue fieldset is added to the form. Beyond that, I was unable to reproduce your issue with the last cue not being rendered in the form. What XML file are you using to reproduce this bug?

Moving forward, what if we iterated the XML datastream with a foreach, ex:

$i = 0;

foreach ($xml->cue as $cue) {
  $fieldset = 'cue_' . $i;
  $form['cues'][$fieldset] = array(
[...]

This to me seems like a safer approach. If you're on board I will get a PR going right away.

MarcusBarnes commented 5 years ago

@BrandonMorr Please proceed as you see fit and we can review. I'll also double check again. We merged because we weren't getting the empty cue fieldset to the form in our testing.

MarcusBarnes commented 5 years ago

@BrandonMorr I've just retested and I'm not getting the extra empty cue fieldset is added to the form based on what I'm doing. If you're using an OH history object where you previously edited using the form, you'll get the extra cue fieldset as per issue https://github.com/Islandora-Labs/islandora_solution_pack_oralhistories/issues/134. Fixing such objects will probably require direct work to fix the transcript XML (not through the edit form).

I note pull-request https://github.com/Islandora-Labs/islandora_solution_pack_oralhistories/pull/140 which may still be an overall improvement - thanks in advance.

BrandonMorr commented 5 years ago

Very strange... when tested this PR I ingested with the changes checked out, here's a screenshot from my VM: screen shot 2019-02-11 at 1 53 48 pm

Regardless, it doesn't seem to be an issue with #140 checked out and that's now been merged (thanks!) so I guess we can move on.

Edit: nevermind, saw the comment reference with the merged badge and jumped the gun haha.

Cheers 🎉