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

Improvements to mediamanager.js and popup-imagemanager.js #25184

Merged
merged 5 commits into from
Jul 2, 2019

Conversation

okonomiyaki3000
Copy link
Contributor

@okonomiyaki3000 okonomiyaki3000 commented Jun 12, 2019

Summary of Changes

  • use jquery's prop function instead of attr
  • properly and consistently encode and decode URI components
  • explicitly use global variables from the global scope

Why do this?

Well, first of all, you should almost always use the prop function instead of attr. What's the difference? One easy way to see is open up your dev tools dom inspector and look at the affected element (for example the action attribute of some forms). When being set with attr, this does not change. When set with prop, it does.

There is a correct way to encode and decode URI components and it does not involve writing your own custom regular expressions.

It's not a good idea to set variables on the global scope and then access them inside this file but, if you're going to do that, at least do it explicitly.

One more thing... Some users have been experiencing an intermittent problem where uploading a file to some nested folder in the media manager actually just sends it to the media base folder. It doesn't happen every time and I haven't been able to duplicate it personally but it does happen. I can't say for sure that these changes will fix the issue but they could be related. Has anyone else experienced this?

Testing Instructions

Use the media manager and popup image manager to navigate around your file system.

Expected result

As usual.

Actual result

As usual.

Documentation Changes Required

 - use jquery's prop function instead of attr
 - properly and consistently encode and decode URI components
 - explicitly use global variables from the global scope
@bayareajenn
Copy link

The issue with uploading images and having them go to the root of /images was happening before 3.9.8. I think perhaps it was after 3.9.6 that the issue began.

What happens is the first time you go to Content -> Media and navigate to a directory within /images (say /images/blog) and upload an image it will work and put it in the correct place. If you go to another place within Media and upload an image, it saves to the root of /images.

If you leave Content -> Media and go to say Content -> Articles or something and then go back to Media, it will work once. Then it saves to the root again.

But I'm not sure I want a whole pop up thing just to resolve that issue. Not even sure how to test it but I'll figure it out later this afternoon or tomorrow when I have time to do more testing.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25184.

@okonomiyaki3000
Copy link
Contributor Author

@bayareajenn Thank you! That is a great clue! You see, when navigating around, only an iframe gets reloaded, the whole page doesn't and the url of the page doesn't change. However, when uploading, you send a post request and get redirected back to the same url + ?folder=wheverver/you/were. I now suspect that the 'folder' part of the url may have something to do with this problem. At least it's something I can test. Of course my users never tell me some useful information like they had uploaded something else previously etc...

@BPBlueprint
Copy link

I checked the 4 files "Improvements to mediamanager.js and popup-imagemanager.js" (and the .min.js) from Jun 12, 2019, but no effect.

Still the same problem.

@ghost
Copy link

ghost commented Jun 14, 2019

@BPBlueprint please mark your test as unsuccessfully > https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results

@BPBlueprint
Copy link

@BPBlueprint please mark your test as unsuccessfully > https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results

@ https://issues.joomla.org/tracker/joomla-cms/25192 ?
It´s marked as "closed"

@ghost
Copy link

ghost commented Jun 14, 2019

@ https://issues.joomla.org/tracker/joomla-cms/25192 ?
It´s marked as "closed"

Its the Issue and get closed if there is a Pull Request. So please mark the Pull Request.

@BPBlueprint
Copy link

@ https://issues.joomla.org/tracker/joomla-cms/25192 ?
It´s marked as "closed"

Its the Issue and get closed if there is a Pull Request. So please mark the Pull Request.

Sorry, I do this for the first time...
Recording the test results (https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results) seems to be within a Joomla!-environment.
The Pull Request seems to be at GitHub (#25184).
So I donot find where/how to mark my test as unsuccessfully.

@ghost
Copy link

ghost commented Jun 14, 2019

Please mark https://issues.joomla.org/tracker/joomla-cms/25184 as unsuccessfully. How to do (please read the doc if needed): https://docs.joomla.org/Testing_Joomla!_patches

@BPBlueprint
Copy link

I have tested this item 🔴 unsuccessfully on 5722fbe

I checked the 4 files "Improvements to mediamanager.js and popup-imagemanager.js" (and the .min.js) from Jun 12, 2019, but no effect.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25184.

@okonomiyaki3000
Copy link
Contributor Author

These are legit improvements whether or not they fix that bug.

@bayareajenn
Copy link

I have tested this item ✅ successfully on 5722fbe

I have applied the patch and navigated around the Media section of the site and all works fine. I'm not able to upload an image to the proper directory, but this has nothing to do with the patch. It's a different bug that this patch isn't meant to solve.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25184.

@bayareajenn
Copy link

@okonomiyaki3000 you might want to edit your original "summary of changes" so that it doesn't include anything about maybe fixing a bug. It doesn't and I don't want it to detour people from testing and giving it an "unsuccessful" result for something it wasn't intended to do. :)

@Quy
Copy link
Contributor

Quy commented Jun 14, 2019

I have tested this item ✅ successfully on 5722fbe


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25184.

@Quy
Copy link
Contributor

Quy commented Jun 14, 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25184.

@joomla-cms-bot joomla-cms-bot added RTC This Pull Request is Ready To Commit PR-staging labels Jun 14, 2019
@okonomiyaki3000
Copy link
Contributor Author

@bayareajenn You're right. I've crossed that part out. An, following up on your clue, I still wasn't able to replicate the issue. It only seems to affect some users sometimes. Is it happening consistently for you?

@Quy
Copy link
Contributor

Quy commented Jun 17, 2019

@okonomiyaki3000 Please read this comment for clarification: #25192 (comment)

@okonomiyaki3000
Copy link
Contributor Author

@Quy Thanks! I'll see what I can do about that.

@okonomiyaki3000
Copy link
Contributor Author

I believe I have a fix.

@okonomiyaki3000
Copy link
Contributor Author

These changes seem to fix it for me. I was finally (maybe) able to duplicate the problem by putting a sleep(10) in the medialist view class. That caused the iframe to load slowly and if I uploaded anything before it finished, the upload would go to the root. After these changes, things went to the right directory.

I had to kind of artificially cause the bug to occur so, if some of you are able to get it in a more consistent way, please test this and see if it solves the problem for you.

@HLeithner HLeithner added this to the Joomla 3.9.9 milestone Jun 21, 2019
@bayareajenn
Copy link

It's not perfect, but it's better. I have one directory where it still saves it in not the place intended. Perhaps my directory is corrupted somehow because in other directories it works.

I had one instance where I uploaded an image to /images/blog and that worked. But when I went into a different subfolder /images/blog/2019 it said the file already existed. Which technically it did but not with that path.

Then I tried uploading an image to just /images/blog and it saved it in images/blog/2019 even though I wasn't there.

But when I did the same thing in images/sampledata with an image and then images/sampledata/parks with the same image, it worked. Thus maybe something is wrong with my folder. So I wasn't sure whether to mark this as an unsuccessful test or successful. Because it's sure a heck of a lot better than what was happening before.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 21, 2019
@ghost ghost added the RTC This Pull Request is Ready To Commit label Jun 21, 2019
@ghost
Copy link

ghost commented Jun 21, 2019

Readded RTC cause bot removed it without a Reason.

@okonomiyaki3000
Copy link
Contributor Author

@bayareajenn Thanks for the details. I'll try that out. By the way, is your 'images' folder configured to be the same as your files folder or some subfolder of it? I'm not sure if it's important, just want a complete picture.

@bayareajenn
Copy link

@bayareajenn Thanks for the details. I'll try that out. By the way, is your 'images' folder configured to be the same as your files folder or some subfolder of it? I'm not sure if it's important, just want a complete picture.

No, the images folder isn't configured to be the same as some subfolder of it. :)

@HLeithner HLeithner merged commit ff2f0a3 into joomla:staging Jul 2, 2019
@HLeithner
Copy link
Member

thx

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 2, 2019
@okonomiyaki3000 okonomiyaki3000 deleted the fix-media-manager branch July 4, 2019 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants