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

允许下载文件的名称与上传文件的名称保持一致 #1744

Merged
merged 20 commits into from
Nov 30, 2024

Conversation

rakuyoMo
Copy link
Contributor

施工中,自认为全部完成后会正式请求 review。

关联 issue #1611 (comment) ,拟作为#1735 的临时解决方案。

我只会一些 ruby 的皮毛,更未涉足过 Ruby on Rails。如果任何人有什么建议或者发现了什么问题,请随时和我沟通,谢谢您的帮助!

Copy link

welcome bot commented Nov 27, 2024

感谢你提交的问题或反馈,我会在有时间的时候来审查代码。
Thanks so much for opening your first PR here!

@icyleaf icyleaf added the wip working in process label Nov 27, 2024
@icyleaf
Copy link
Member

icyleaf commented Nov 27, 2024

一点想法:

  1. 把选项放到 release 每上传一次都要传一次是不是太麻烦了,不如放到上一层 channel 会更好一些。
  2. 原 issue 如果也提 PR 那岂不是又要多一个字段,而且都设置 true 怎么办,不如改成一个枚举字段 :D

现在应用的三层结构在定义一些设置有些繁琐,太粗太细都不合适,之前有计划规划设置 policy 模板,可以随意 apply 到任意一层,鉴于时间精力也不盈利的情况下一直在鸽

@Issues-translate-bot
Copy link

Bot translate issue to English automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


Some thoughts:

  1. Is it too troublesome to put the option in release? It has to be passed every time it is uploaded. It would be better to put it in the upper channel.
  2. If the original issue also raises a PR, wouldn’t it require one more field, and both should be set to true? Why not change it to an enumeration field :D

The three-layer structure of the current application is a bit cumbersome to define some settings, and it is not suitable to be too thick or too detailed. I had previously planned to set up a policy template, which can be applied to any layer at will. In view of the fact that time and energy are not profitable, I have been pigeonholing it.

@rakuyoMo
Copy link
Contributor Author

一点想法:

  1. 把选项放到 release 每上传一次都要传一次是不是太麻烦了,不如放到上一层 channel 会更好一些。
  2. 原 issue 如果也提 PR 那岂不是又要多一个字段,而且都设置 true 怎么办,不如改成一个枚举字段 :D

很好的建议,我会尝试将 use_original_filename 改为一个枚举并放到 channel 层。

目前打算提供 “原文件名” 和 “系统内置” 两个选项,“系统内置” 为默认值。本 pr 不考虑新增原 issue “完全自定义” 的情况。

@Issues-translate-bot
Copy link

Bot translate issue to English automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


Some thoughts:

  1. Isn’t it too troublesome to put the option in release? It has to be passed every time it is uploaded. It would be better to put it in the upper channel.
  2. If the original issue also raises a PR, wouldn't it require one more field, and set both to true? Why not change it to an enumeration field :D

Good suggestion, I will try to change use_original_filename to an enum and put it in the channel layer.

It is currently planned to provide two options: "Original file name" and "System built-in", with "System built-in" being the default value. This PR does not consider the "completely customized" situation of adding the original issue.

rakuyoMo added a commit to rakuyoMo/zealot that referenced this pull request Nov 27, 2024
1. Only control fields are added, and the file name logic for downloading has not yet been implemented
2. Swagger is not included
3. Channel information display is not included
@rakuyoMo rakuyoMo marked this pull request as ready for review November 27, 2024 10:14
@rakuyoMo
Copy link
Contributor Author

rakuyoMo commented Nov 27, 2024

@icyleaf 我自认为完成了相关逻辑,现在可以开始 review 了。

目前开启相关设置后,下载文件的文件名和上传时保持一致。未开启设置时依然和之前逻辑一致。

app/models/channel.rb Outdated Show resolved Hide resolved
@Issues-translate-bot
Copy link

Bot translate issue to English automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@icyleaf I think I have completed the relevant logic and can now start reviewing.

app/models/release.rb Outdated Show resolved Hide resolved
Copy link
Member

@icyleaf icyleaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

会 Ruby 基本就会 Rails,别忘了 Rails 可以从入门到 IPO 的 slogan 😆

app/models/channel.rb Outdated Show resolved Hide resolved
app/models/release.rb Outdated Show resolved Hide resolved
app/models/channel.rb Outdated Show resolved Hide resolved
config/locales/simple_form/simple_form.zh-CN.yml Outdated Show resolved Hide resolved
@icyleaf icyleaf removed the wip working in process label Nov 28, 2024
config/locales/zealot/en.yml Outdated Show resolved Hide resolved
config/locales/zealot/zh-CN.yml Outdated Show resolved Hide resolved
Copy link
Member

@icyleaf icyleaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

修改完这个就可以合并了 😄

Copy link
Member

@icyleaf icyleaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@icyleaf icyleaf merged commit 881b341 into tryzealot:develop Nov 30, 2024
2 checks passed
Copy link

welcome bot commented Nov 30, 2024

感谢贡献 PR 来支持 Zealot! 🎉
Congrats on merging your first pull request here! 🎉 How awesome!
Congrats!

@rakuyoMo rakuyoMo deleted the feature/original-file-name branch November 30, 2024 02:49
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.

3 participants