Skip to content
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

post 6823 edit file form update fixes #7126

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Jul 23, 2020

What this PR does / why we need it: fixes bugs introduced in by #7045

Which issue(s) this PR closes:

Closes #7123

Special notes for your reviewer: I picked the contentOfHttpPanel to fix the direct upload issue (when the select files widget is updated, the directupload script that is outside that (above in the code) needs to run again and wasn't. Any other element outside the script could work. For the fix of the button updates, I changes the name of the element from filesButtons to completionButtons primarily because an empty element with that name is now in the dataset.xhtml but the name could stay as it was.(I originally put the empty panel around the button bar in the dataset form (in which case a name change would be more helpful) but I updating those buttons (in dataset create mode) isn't needed.)

Suggestions on how to test this: Current production - using direct upload and uploading a file leaves the 'select new files' box with upload and cancel buttons showing. The button at the bottom of the page remains as 'Done' rather than updating to 'Save/Cancel' . (Same behavior after the first upload to a file store, but the 'Done' button updates to Save/Cancel after a second upload of additional file(s)). With the fix, direct-upload and normal upload to s3 or files (testing just one of these two should be OK) in dataset create and edit mode should always work - direct upload shows a normal select files box after a single file is uploaded and the 'Done' button changes in edit for all cases after one upload, and the dataset create form still doesn't scroll to the top of the page (just keeps the select files box in view)).

Does this PR introduce a user interface change? If mockups are available, please link/include them here: only restores missing functionality from prior to #7045.

Is there a release notes update needed for this change?: no

Additional documentation:

@coveralls
Copy link

coveralls commented Jul 23, 2020

Coverage Status

Coverage remained the same at 19.613% when pulling dc19fdc on GlobalDataverseCommunityConsortium:IQSS/7123_edit_file-form_updates into 4525f94 on IQSS:develop.

@@ -282,7 +282,7 @@

<p:commandButton id="updateEditDataFilesButtonsForUpload" action="#{EditDatafilesPage.uploadFinished()}" update="datasetForm:editDataFilesButtons" rendered="#{showFileButtonUpdate}" style="display:none"/>
<p:commandButton id="updateEditDataFilesButtonsForDelete" action="#{EditDatafilesPage.deleteFilesCompleted()}" update="datasetForm:editDataFilesButtons" rendered="#{showFileButtonUpdate}" style="display:none"/>
<p:commandButton id="AllUploadsFinished" action="#{EditDatafilesPage.uploadFinished()}" update="datasetForm:fileUpload,datasetForm:dropBoxUserButton,datasetForm:uploadMessage,datasetForm:rsyncPanel,datasetForm:filesCounts,datasetForm:filesTable" oncomplete="javascript:bind_bsui_components();javascript:uploadWidgetDropMsg();" style="display:none"/>
<p:commandButton id="AllUploadsFinished" action="#{EditDatafilesPage.uploadFinished()}" update="datasetForm:completionButtons,datasetForm:contentOfHttpPanel,datasetForm:fileUpload,datasetForm:dropBoxUserButton,datasetForm:uploadMessage,datasetForm:rsyncPanel,datasetForm:filesCounts,datasetForm:filesTable" oncomplete="javascript:bind_bsui_components();javascript:uploadWidgetDropMsg();" style="display:none"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the addition of the contentOfHttpPanel to the update here make the fileUpload component redundant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe? It was there when @Form was in it too, so I wasn't sure - if it was redundant then too then maybe it can be removed. Some of the other elements could also be redundant - I didn't check.

@@ -117,7 +117,7 @@
</div>
</div>
<div class="col-xs-7 text-right">
<p:outputPanel id="filesButtons">
<p:outputPanel id="completionButtons">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a duplicate ID of the new completionButtons outputPanel added to the dataset?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~yes, but the editFileFragment is either embedded in the dataset.xhtml or filesFragement.xhtml, but not both, so I think there is only ever one element with this id. Having them the same is really the point - as the AllUploadsFinished command tries to update 'completionButtons' and fails if it can't find it, which is what happened when I tried this without having the empty element in dataset.xhtml with this name. If there's a way to update an element only if it exists, that should work instead.

@djbrooke djbrooke added this to the Dataverse 5 milestone Jul 24, 2020
@sekmiller sekmiller self-assigned this Jul 27, 2020
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Code Review 🦁 to QA 🔎✅ Jul 28, 2020
Copy link
Contributor

@sekmiller sekmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that all you needed to do was to add the contentOfHttpPanel to the update of allUploadsFinished. I don't think the renaming of the buttons on the dataset page and editdatafiles was necessary, but it doesn't' hurt anything - though it might make future merges more fun.

@qqmyers
Copy link
Member Author

qqmyers commented Jul 28, 2020

@sekmiller - adding the buttons to the update was needed to fix the issue of updating the button from 'Done' to Save/Cancel - they're outside the contentOfHttpPanel. Since they only existed in filesFragment and didn't exist when you're in dataset create mode, I also had to add a panel with the same name in dataset.xhtml. The name change itself is gratuitious and could be reverted - it doesn't hurt to have an empty panel in dataset.xhtml with id filesButtons (just thought that was an odd name when I originally put the panel around the dataset create button panel (which doesn't have anything to do with filesButtons). So - if it helps to change the name back, it would be easy to do.

@sekmiller
Copy link
Contributor

@qqmyers I thought that using " update=":datasetForm:filesTable, @([id$=filesButtons])" meant that it didn't matter if the id occurred in the dataset page in create mode, and it seemed to work fine in 6823, unless it was something that I missed due to direct upload. Either way I think it might be helpful to change the name back to filesButtons to help avoid merge conflicts as long as it's easy. Thanks!

@qqmyers
Copy link
Member Author

qqmyers commented Jul 28, 2020

@sekmiller - I'm confused - there was no @([id$=filesButtons]) before or after #6823? Before it, the @Form refreshed the buttons without specifying their id (but it also refreshed the whole dataset creation page moving you back to the top). If @([id$=filesButtons]) will refresh the buttons only if that id exists, then that would mean there's no need for that element in dataset.xhtml and I can remove that as well. Is that the case?

@sekmiller
Copy link
Contributor

I believe so, yes.

@qqmyers
Copy link
Member Author

qqmyers commented Jul 28, 2020

@sekmiller - I see that the @([id$=filesButtons]) was/is used in the file input's update method, so per your suggestion, I've just used that in the command as well. Thanks!

@kcondon kcondon self-assigned this Jul 29, 2020
@kcondon kcondon merged commit 746d7b6 into IQSS:develop Jul 29, 2020
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA 🔎✅ to Done 🚀 Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Page update issues after #6823
6 participants