Change to use an unified constant for max media attachments per status

[ileodo]

Currently, there are multiple places referring a hard coded magic number 4 as the max media attachment per status. This PR is creating a constant in MediaAttaments Mode to unifying the definition.

both JS and ruby will use the same definition.

[mjankowski]

Looks good overall. Some followup ideas to add to this PR or afterwards:

• The PostStatusService spec does check the 4 attachment limit -- this could be update to stub_const this new constant limit and simplify the spec (or at minimum, change the hard-coded 4 there to just reference the constant).
• The UpdateStatusService is missing this spec coverage - could be added in similar way.

[ileodo]

Thanks @mjankowski could you please help to enable the CI action for this PR?
Also do you want me to follow up on the two points? I'm not super familiar with Ruby on Rails, but I can give a try (it might take longer)
or you are fine to leave the for the future.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 85.00%. Comparing base (86500e3) to head (b4ffd04).
Report is 211 commits behind head on main.

❗ Current head b4ffd04 differs from pull request most recent head b21618c. Consider uploading reports for the commit b21618c to get more accurate results

Files	Patch %	Lines
app/services/update_status_service.rb	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #29073      +/-   ##
==========================================
- Coverage   85.01%   85.00%   -0.02%     
==========================================
  Files        1059     1058       -1     
  Lines       28277    28282       +5     
  Branches     4538     4538              
==========================================
+ Hits        24040    24041       +1     
- Misses       3074     3078       +4     
  Partials     1163     1163              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.

[github-actions]

This pull request has resolved merge conflicts and is ready for review.

[renchap]

Looks good!

Did you try locally to increase the value locally and check that both the UI and the API support the new value?

[ileodo]

Looks good!

Did you try locally to increase the value locally and check that both the UI and the API support the new value?

thanks! yes, I did tested it locally with changing the const number.

[renchap]

It looks like this is breaking the end-to-end tests in the admin panel, specifically this one: https://github.com/mastodon/mastodon/blob/main/spec/system/report_interface_spec.rb#L23

Can you please look at this and fix it?

It breaks the admin panel

[ClearlyClaire]

I think the admin panel failure is because the Media Gallery gets mounted out of a redux context there.

[ileodo]

looks like there is some issue with the current main branch, under development mode (in docker), admin@localhost is not approved.. so I can't replicate and test the change, any idea about the "approved" issue?

[renchap]

You can approve it from the CLI using bin/tootctl modify <username> --approved

[ileodo]

@ClearlyClaire Could you please help with fixing the media gallery in the report view? I'm not very familiar with how redux is used in mastodon, but definitely want to push for this PR. Thanks.

[renchap]

The Redux store is not initialised at all when we render independent React components in a page (in your case, <MediaContainer>).

A way to solve this is to properly initialise Redux when mounting a MediaContainer so it has access to the Redux state. This is probably overkill in this specific case as you are only interested in one specific field from the instance endpoint.
I did this here, but I am not sure this is the best way to handle your issue: renchap@534ff0c

Another way to do this would be to somehow detect if we are in a Redux context, and if not, fetch this value and pass it to the component, but I do not think we do this in any other place?

[ileodo]

great. CI passed!

[ileodo]

bouncing this up.

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.

[github-actions]

This pull request has resolved merge conflicts and is ready for review.

[ileodo]

This PR has been here for ages.. can any one review and see if it can get merged??

[ClearlyClaire]

The authoring part is fine, but I'm not sure about the display part, for the following reasons:

1. limiting the number of media in a gallery client-side seems unnecessary if the server also enforces this limit (note that there is a caveat in that very old versions of Mastodon did not limit the number of attachments stored)
2. similarly, the request for the server configuration (fetchServer()) could be avoided
3. design-wise, the media gallery component does not gracefully handle more than 4 attachments: when more than 4 attachments are attached, only the first two will be displayed, and the remaining ones will be part of the DOM, but below the first two, overflowing the component and thus hidden; this probably needs bigger design changes for such a feature to be useful

[ileodo]

@ClearlyClaire I agree with what you said that there will be more work required if we want to support more than 4 media per post. This is why I made it clear in the PR title and description that the intention of this PR is just refactoring to have a constant indicate the limit instead having magic number 3/4 everywhere.

Are we fine with the refactoring?

[ClearlyClaire]

Makes sense, but I think this would be easier done by:

• removing the take(4) from the front-end entirely
• possibly enforcing this server-side instead in REST::StatusSerializer

This would simplify the client-side code greatly and avoid doing an extra request to the API.

Further down the line, the UI can be changed to handle arbitrary many attachments in a more graceful way, and the limit can be lifted from the serializer.

[ileodo]

@ClearlyClaire The client side need to be aware of this limit, because it's not only for display but also for uploading:

1. The WebUI currently have the logic to check the limit when user uploading medias.
2. Other clients (like iOS apps, e.g. IceCube) need to know the limit so that they can limit the same in the media picker widgets.

And still, I believe this PR is helping to move to a better place instead of making it worse. as in the master branch now, the backend is using a constant but magic number 3/4 everywhere in UI code

[ClearlyClaire]

The client side need to be aware of this limit, because it's not only for display but also for uploading

I'm not disputing that, what I meant is that we can remove complexity from app/javascript/mastodon/components/media_gallery.jsx and app/javascript/mastodon/containers/media_container.jsx by not doing the checks here.

The changes in app/javascript/mastodon/actions/compose.js and app/javascript/mastodon/features/compose/containers/upload_button_container.js are fine.

[ileodo]

@ClearlyClaire updated as per your comments

[ileodo]

@ClearlyClaire may I know is it good to be merged now?

[ileodo]

can someone have a look at this PR? it has been there for ages...

[ClearlyClaire]

Please keep in mind that the Mastodon core team is very small, and has a lot of work to do (like handling the recent security release).

The code changes appear fine, although one of my suggestions is missing:

possibly enforcing this server-side instead in REST::StatusSerializer

I think this is important because in the absence of a media gallery redesign, having more than 4 media attachments results in a strictly less usable interface as only considering the 4 first media attachments. Furthermore, old versions of Mastodon used to not discard media attachments above 4 attachments, so viewing such posts would result in a degraded experience.

I will try to tackle these server-side changes myself soon, and I think this PR can be merged as soon as this is implemented.

But to be truly useful, the media gallery needs to be redesigned to gracefully handle more than 4 attachments.

[ileodo]

thanks, apart form #30932, anything you think we need for this PR? @ClearlyClaire