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

Uploader plus messes up with the initialization of some form controls #127

Closed
yregaieg opened this Issue Oct 19, 2017 · 22 comments

Comments

Projects
None yet
3 participants
@yregaieg
Collaborator

yregaieg commented Oct 19, 2017

When you use some form control that need to be initialized as soon as it gets added to the dom (ie: the alfresco-value-assistance-addon); This process sometimes become tricky when you either have uploaded a document using uploader-plus before the last page refresh or when you are trying to upload multiple documents at the same time.

This problem only occurs when the formcontrol in question is part of the default (first in the list) form authorized in the current upload folder and the form in question happens to have been the last form to be used after the last page refresh.

If you have multiple upload forms available, you could switch to an other form and back and the issue is solved.

A workaround is to refresh the page and upload next document, but it is very painful.

@yregaieg

This comment has been minimized.

Collaborator

yregaieg commented Oct 19, 2017

@douglascrp already fixed this issue but will need to clean up my implementation and test all edge cases before I make a PR

yregaieg added a commit to yregaieg/uploader-plus that referenced this issue Oct 19, 2017

@douglascrp

This comment has been minimized.

Collaborator

douglascrp commented Oct 20, 2017

Closing

@douglascrp douglascrp closed this Oct 20, 2017

@softwareloop

This comment has been minimized.

Owner

softwareloop commented Jan 29, 2018

Hi, I have tested cf2c2dd and I believe it breaks the "Apply the same metadata set to all files" functionality. To replicate the problem:

  • set up an uploader-plus folder from the admin panel
  • visit the folder
  • upload two files, either using the upload button or by drag&drop
  • click on "Apply the same metadata set to all files"
  • enter any required fields
  • click ok

The following problem is observed:

  • the header of the upload popup stays on, see image below
  • one has to click on the "X" button to make it go away
  • the files are not uploaded

uploader-plus-error

I have tested on Alfresco versions 201704, 201701, 201604, 201602 always using Chrome. @yregaieg can you confirm you see the same?

The problem may be related to the code:

                while (formNode.hasChildNodes()) {
                    formNode.removeChild(formNode.lastChild);
                }

I'm re-opening this issue so we can discuss some changes.

@softwareloop softwareloop reopened this Jan 29, 2018

@yregaieg

This comment has been minimized.

Collaborator

yregaieg commented Jan 29, 2018

@softwareloop I confirm being able to reproduce this with the scenario described.
I will try to reserve some time during this week to re-test #128 (comment) and see if @douglascrp implementation is any better in this regard.

@softwareloop

This comment has been minimized.

Owner

softwareloop commented Jan 29, 2018

I've tested @douglascrp 's https://github.com/douglascrp/uploader-plus/tree/issue-127-B branch and it is also affected.

I manually tested the most recent commits to be sure. The last commit that is NOT affected by the problem is 32e0842

@yregaieg

This comment has been minimized.

Collaborator

yregaieg commented Jan 29, 2018

@yregaieg

This comment has been minimized.

Collaborator

yregaieg commented Jan 29, 2018

@softwareloop the reason behind this bug is that that we have removed all children from the form after the first iteration (ie: after building the first upload payload) and so the consequent files fail silently (because the form elements are no longer available).
Not sure if the index of the item is available in the closure or not, but if it is I think we should do one test more and only remove form children if the option for using the same metadataset for all documents is not active, or if it is active and we are at the very last iteration.

@softwareloop

This comment has been minimized.

Owner

softwareloop commented Jan 30, 2018

I'm trying to reconstruct the data flow, also looking at https://github.com/softwareloop/uploader-plus/blob/master/docs/alfresco-uploader-plus-diagrams.pdf although the diagram is only partial.

Looking at dnd-upload-plus.js:

  1. the entry point is _spawnUploads at dnd-upload-plus:46
  2. if the folder is regular (not an uploader-plus folder), it will call _spawnUploads of the superclass
  3. if it is an uploader-plus folder, it will call showMetadataDialog
  4. in showMetadataDialog, if there are no more files to process, it will call _spawnUploads of the superclass
  5. if there is a file to process, if will fire the "change" event on the type selector
  6. the event is picked up by uploader-plus-mixin:120, method onContentTypeChange
  7. onContentTypeChange will do an ajax call to retrieve the form from share
  8. when the form is received, onMetadataFormReceived is called
  9. onMetadataFormReceived takes care of:
    • calling onReady to initialise the form
    • setting up the Next/ok button listener
    • resetting the "use same metadata" checkbox and listener
  10. at this point the user is in control, fills in some data and clicks on next/ok
  11. The next/ok button listener is onMetadataSubmit at uploader-plus-mixin:286
  12. This will validate/process the metadata and, if selected, apply the same metadata to the remaining files (@douglascrp can you confirm?)
  13. onMetadataSubmit finally call showMetadataDialog and the cycle is repeated from step 4

@yregaieg, do you see a place where the DOM/form cleanup can take place? Maybe in step 4 (showMetadataDialog) or step 12 (onMetadataSubmit).

Feel free to correct/improve my analysis of the data flow.

@yregaieg

This comment has been minimized.

Collaborator

yregaieg commented Jan 30, 2018

As far as I can remember, I couldn't delay it to a later spot nor advance it to an earlier spot, and it needed to be run twice for some uploaders so that it would be supported on other uploaders as well (that last part, I am not sure if I was able to avoid it eventually ... as I had a client in my back urging me for a deployment)

We can definitely limit that cleanup to when we are not using the option for using the same metadata set for multiple uploads, either way, the bug it solves doesn't show up unti you try to fill in the form a second time... Or we could go further, and pass on the status along to the method for processing the upload (in case the information is not yet available in the closure) to only do the cleanup after processing the very last document to be uploaded when that option is on

@softwareloop

This comment has been minimized.

Owner

softwareloop commented Jan 30, 2018

@yregaieg: Is the DOM clean-up intended to be executed once per upload batch, or for each uploaded file? By batch I mean e.g. when you drag&drop three files in one go.

I'm trying to understand if _resetGUI() could be the right place for the clean up.

Another thought: forms have a formUi.onReady method for initialisation. Do they also have a method for cleaning up? I just don't know/remember the surf API so well.

@softwareloop

This comment has been minimized.

Owner

softwareloop commented Jan 31, 2018

@yregaieg I have time this week to do some overall testing of the plugin and make an official release (v1.6). Is it ok if I revert cf2c2dd and then re-apply it on a separate branch, so we can work on this issue in the next 1.7-SNAPSHOT?

@yregaieg

This comment has been minimized.

Collaborator

yregaieg commented Jan 31, 2018

@softwareloop

This comment has been minimized.

Owner

softwareloop commented Jan 31, 2018

Thanks

@softwareloop

This comment has been minimized.

Owner

softwareloop commented Feb 1, 2018

I have released v1.6 as discussed. I have now pushed a new branch https://github.com/softwareloop/uploader-plus/tree/issue-127 and applied the previous commit 3365102

@yregaieg

This comment has been minimized.

Collaborator

yregaieg commented Feb 1, 2018

@douglascrp

This comment has been minimized.

Collaborator

douglascrp commented May 30, 2018

Hello guys.
I will have to look at this issue, as we are going to use uploader-plus in a new project we are working right now, and we are facing the problem described here.

Is there any chance that you have tried something to fix it? Or should I simply get the discarded commits and try to make it work?

@softwareloop

This comment has been minimized.

Owner

softwareloop commented May 31, 2018

No I haven't tried anything. The "issue-127" branch contains the commits that were discarded from master.

@douglascrp

This comment has been minimized.

Collaborator

douglascrp commented Jun 1, 2018

@softwareloop Ok, no problem.
I am going to work on this issue right now.

@douglascrp

This comment has been minimized.

Collaborator

douglascrp commented Jun 1, 2018

Any chance that you guys can review and test this new version?
I was playing with it, trying to combine a lot of different steps, using both one-by-one and use same metadata, and judging by my tests, everything seems to be working well.

I am going to deploy this version in my development server and ask the users to test it too.

@douglascrp

This comment has been minimized.

Collaborator

douglascrp commented Jun 3, 2018

It is not working... I am doing more tests.

@douglascrp

This comment has been minimized.

Collaborator

douglascrp commented Jun 3, 2018

Ok guys, I have made one more change, and this time we were not able to make it fail.
Please, if you can, execute some tests before we merge this.

@softwareloop

This comment has been minimized.

Owner

softwareloop commented Jul 19, 2018

@douglascrp, @yregaieg, you've done a great job to resolve this tough issue. Thank you. @douglascrp
has helped me set up a test environment with alfresco-value-assistance, a custom content model and uploader-plus, so I could test compatibility from Alfresco 201602 to 201707. Everything worked great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment