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

Projects
None yet
7 participants
@okonomiyaki3000
Copy link
Contributor

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

Improvements to mediamanager.js and popup-imagemanager.js
 - 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

This comment has been minimized.

Copy link

commented Jun 13, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@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

This comment has been minimized.

Copy link

commented Jun 14, 2019

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.

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

@BPBlueprint

This comment has been minimized.

Copy link

commented Jun 14, 2019

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

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.

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.

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

commented Jun 14, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

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

@bayareajenn

This comment has been minimized.

Copy link

commented Jun 14, 2019

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

This comment has been minimized.

Copy link

commented Jun 14, 2019

@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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

RTC


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

@okonomiyaki3000

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

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

@okonomiyaki3000

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

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

@okonomiyaki3000

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

I believe I have a fix.

@okonomiyaki3000

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

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

This comment has been minimized.

Copy link

commented Jun 21, 2019

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 label Jun 21, 2019

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Readded RTC cause bot removed it without a Reason.

@okonomiyaki3000

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

@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

This comment has been minimized.

Copy link

commented Jun 24, 2019

@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

4 checks passed

Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@HLeithner

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

thx

@joomla-cms-bot joomla-cms-bot removed the RTC label Jul 2, 2019

@okonomiyaki3000 okonomiyaki3000 deleted the okonomiyaki3000:fix-media-manager branch Jul 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.