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

Media Browser doesn't display popup to select a media source for user profile photos in MODX 3.0 #16510

Closed
SnowCreative opened this issue Jan 12, 2024 · 7 comments · Fixed by #16515
Assignees
Labels
bug The issue in the code or project, which should be addressed.

Comments

@SnowCreative
Copy link
Contributor

Bug report

Summary

Media browser that popups up when selecting a photo for a user profile only displays the default media source

Step to reproduce

In MODX 3 site, edit your profile, and click the icon to select a user photo.

Observed behavior

There is no popup to select a media source. Only the folder for the default media source is displayed.
Screen Shot 2024-01-12 at 1 20 24 PM

Expected behavior

The Media Browser popup to select a media source should be displayed, just like it is everywhere else the media browser is used.
Screen Shot 2024-01-12 at 1 22 10 PM

Environment

MODX 3.0.4-pl

@SnowCreative SnowCreative added the bug The issue in the code or project, which should be addressed. label Jan 12, 2024
@smg6511
Copy link
Collaborator

smg6511 commented Jan 15, 2024

Ok, so after a little research on this I found that the behavior has always been this way dating back to the introduction of profile photos nearly 10 years ago. The source combo was purposely hidden and a system setting (photo_profile_source) was added into the conditional that defines which source the media browser should use (in the context of profile photo insertion):

{
    id: 'modx-user-photo'
    ,fieldLabel: _('user_photo')
    ,name: 'photo'
    ,xtype: 'modx-combo-browser'
    ,hideFiles: true
    ,source: MODx.config['photo_profile_source'] || MODx.config.default_media_source
    ,hideSourceCombo: true
    ,anchor: '100%'
}

@rtripault was the author of the original code and may be able to shed some light on that decision (maybe ... it's been an awfully long time!).

Before proceeding on any changes, the question arises: Is this a bug, a missing feature, or a move made (to limit the source to the two settings-defined options) for good reason that we should stick with?

@SnowCreative
Copy link
Contributor Author

Ah, I was hoping there was a system setting but couldn't find it. That kind of makes sense, but I don't know why just using the normal media manager setup wouldn't work. I assume access to media sources would be limited by permissions set on each source, as it is in every other context of using the media manager. If anything is preventing that from being the case for profile photos, then I can see why a system setting would be needed. What would be nicest is to make the system setting optional: if something is entered there, then only show that media source; and if it's empty then show the full list in the browser.

@rtripault
Copy link
Contributor

That's an old one indeed!

Not sure what was the old me thinking back then... i guess ease of being able to render pictures in the frontend then... but having ability to pick the source definitely makes sense!

With current implementation, you might end up having a media source configured which some user might not have access too.

@smg6511
Copy link
Collaborator

smg6511 commented Jan 15, 2024

Alright, sounds like there's no reason not to add the sources dropdown. It'll involve a handful of adjustments and additions, which I could dig into in the next week or two...

@smg6511 smg6511 self-assigned this Jan 16, 2024
@smg6511
Copy link
Collaborator

smg6511 commented Jan 18, 2024

After thinking on this a little more and batting it around with @opengeek, I've concluded that the best course of action would be:

  1. Show a disabled source dropdown in the media browser. This would keep the UI more consistent; I could also add a little hint via a tooltip or simply via a title attribute re the ability to customize the source via the photo_profile_source system setting. (That setting has always been available for choosing a photo source other than the default_media_source, but never installed.); and
  2. Add (install) the photo_profile_source setting into the predefined system settings. Right now, that special source is taken into account but, because it was never made available otherwise, one would never know it was there to take advantage of.

This should offer plenty of granularity via user group, context, and/or user settings.

BTW, the reason to not enable the sources dropdown: If we did that, we'd need to persist that source via a new column in the user profile; not a big deal, but it's really not the type of data we want to be storing there.

@SnowCreative
Copy link
Contributor Author

That setting has always been available for choosing a photo source other than the default_media_source, but never installed

Well no wonder I coudn't find it!

@smg6511
Copy link
Collaborator

smg6511 commented Jan 19, 2024

BTW, in the submitted PR I elected to not add a disabled source dropdown as, after another survey of how the Media Browser appears in other instances, it was more consistent to hide it than not.

opengeek pushed a commit that referenced this issue Mar 25, 2024
### What does it do?
Adds a long-missing setting for specifying user's profile photo media
source. Also, moves up (and tweaks) the associated Lexicon entries.

### Why is it needed?
The logic has been present in the backend to specify a Media Source
other that the default for user profile photos since they were
introduced nearly 10 years ago. However, the system setting was never
added to the build, making it virtually unknown that this functionality
existed.

### How to test
Run `_build/transport.core.php` to install the new setting. Then search
for `photo_profile_source` in the system settings, change its value, and
verify that the Media Source shown in the Media Browser (when
editing/adding a user profile photo) matches that which you specified in
the setting.

### Related issue(s)/PR(s)
Resolves #16510
opengeek pushed a commit that referenced this issue Mar 25, 2024
### What does it do?
Adds a long-missing setting for specifying user's profile photo media
source. Also, moves up (and tweaks) the associated Lexicon entries.

### Why is it needed?
The logic has been present in the backend to specify a Media Source
other that the default for user profile photos since they were
introduced nearly 10 years ago. However, the system setting was never
added to the build, making it virtually unknown that this functionality
existed.

### How to test
Run `_build/transport.core.php` to install the new setting. Then search
for `photo_profile_source` in the system settings, change its value, and
verify that the Media Source shown in the Media Browser (when
editing/adding a user profile photo) matches that which you specified in
the setting.

### Related issue(s)/PR(s)
Resolves #16510
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue in the code or project, which should be addressed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants