-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
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.
Hey Andrés, I think this looks good. However, there is a lot of code duplication. Most of it was probably already present, but I think it would be good if we can start correcting this. Can you please try to:
- Use template snippets for common pieces, like the upload/URL selection.
- Parametrize existing javascript methods instead of copying and editing.
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.
Hi Andrés,
I've added more comments. Besides them I have one more request: is it possible to verify the format of the file after downloading them to the server? As we do for the files that are uploaded from the client.
I've got also had these issues: when displaying the page below, no option is selected. Please make that the "Upload files" option is selected by default.
Also, the footer hides the "Start" button after selecting the "Upload files" option:
I believe that, since all files are sequentially downloaded in the server side, it would be nice if we have some kind of visual feedback on how many files are processed and how many are left, instead of the "Processing" modal.
Done.
Done.
Done here.
Done. |
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.
Hi Andrés,
I reviewed the code and it seems ok to me. There is a lot of repetition still, but we can continue working to remove this gradually.
When testing I've found two issues:
-
I provided the URLs for a release package and a record package together in the Combine Packages feature. When submitting there is no visual feedback that says something went wrong, although there are errors in the server console. The resulting file is a package with the release package only.
-
When reloading the Combine Packages page I still see one of the URLs I selected before in the single input box that is visible. I'm not sure if this is because of my browser (firefox) but it would be nice to ensure this field is blank when reloading the page.
@jpmckinney would you like to review this before merging? |
I haven't reviewed – looks like you did a thorough review! I did "Squash and merge" since there were a lot of commits addressing the same issues, which made history a bit unclear. |
#42