-
Notifications
You must be signed in to change notification settings - Fork 38
Color indication for user upload limits (resolve #677) #730
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
base: master
Are you sure you want to change the base?
Color indication for user upload limits (resolve #677) #730
Conversation
|
Мне нравится. Новые цвета я бы, может, ещё поподбирал - кажутся ярковаты. Но пусть потом лучше модераторы предлагают варианты. |
ba6d907 to
d3698b9
Compare
|
Поменял цвета на то что предложил в issue и собрал в один коммит |
kabalin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Привет! Спасибо за PR!
Добавлен фильтр "Лимит пользователя - Любой/ Меньше Х" , позволяющий показывать фотографии только пользователей с любым лимитом или меньше определенного числа
может быть выбрана только одна из опций
дефолтное значение лимита 75
дефолтная галочка "Любой"
Мне кажется "Любой" - лишняя опция, если я ничего не упускаю. Мы по умолчанию принимаем что лимит "любой" (т.е. не применен, как это работает сейчас), а если выбран "Меньше", тогда применяется лимит. Это упростит код.
controllers/photo.js
Outdated
|
|
||
| if (user.rules && _.isNumber(user.rules.photoNewLimit)) { | ||
| canCreate = Math.max(0, Math.min(user.rules.photoNewLimit, maxNewPhotosLimit) - pfcount); | ||
| limit = Math.min(user.rules.photoNewLimit, maxNewPhotosLimit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не критично, но в принципе можно во всех условиях использовать return так как переназначения переменной больше не будет.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Поменял. Правда из-за стандарта теперь оно в 2 раза длиннее
| let aggQuery = [ | ||
| { $lookup: { from: 'users', localField: 'user', foreignField: '_id', as: 'user_info' } }, | ||
| { $match: query }, | ||
| { $set: { photoNewLimit: { $sum: '$user_info.rules.photoNewLimit' } } }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почему используется $sum, там же значение переменной - число?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не очень знаток mongodb, но sum из-за того, что это агрегированный запрос.
Без $sum не работает, а что там другое может быть ума не приложу.
| { $set: { photoNewLimit: { $sum: '$user_info.rules.photoNewLimit' } } }, | ||
| { $set: { pcount: { $sum: '$user_info.pcount' } } }, | ||
| { $set: { pfcount: { $sum: '$user_info.pfcount' } } }, | ||
| { $set: { ranks: { $first: '$user_info.ranks' } } }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$first не совсем понятно почему нужно только первое значение в массиве ranks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это не первое значение в массиве, а первый массив ranks.
Запись юзера все равно будет одна.
Нужно как я понял использовать хоть какую-то функцию из-за агрегата.
| { $set: { photosLimit: { $sum: { $switch: { | ||
| branches: [ | ||
| { case: { $gt: ['$photoNewLimit', 0] }, then: { $min: ['$photoNewLimit', maxNewPhotosLimit] } }, | ||
| { case: { $in: [{ $max: '$ranks' }, ['mec_silv', 'mec_gold']] }, then: maxNewPhotosLimit }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$max к строке применяется если я правильно понял? (до этого присвоили первое значение из массива { $set: { ranks: { $first: '$user_info.ranks' } } })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$first в том случае возвращает первый массив а не первое значение в массиве.
Да, тут небольшой чит из-за тех значений, что есть в справочнике, очень удобно получилось.
Они прям в нужном порядке сортируются с помощью $max "adviser","mec","mec_gold","mec_silv" (у последних двух одинаковый лимит)
| } | ||
| } | ||
| } | ||
| } else if (filterParam === 'l') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут комментарии в этой части diff не помешали бы, тяжело понять логику.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Добавил
views/module/photo/gallery.pug
Outdated
| // /ko | ||
| //ko foreach: photos | ||
| .photoPreview.withStatus.withInfo.fringe(data-bind="attr: {title: title}, css: ($parent.feed() ? 'trans ' : '') + 's' + $data.s") | ||
| .photoPreview.withStatus.withInfo.fringe(data-bind="attr: {title: title}, css: ($parent.feed() ? 'trans ' : '') + 's' + $data.s + (($parent.auth.iAm.role() >= 5 && $data.status.num <= 2 ) ? ( $data.userPhotoLimitMax <= 10 ? ' photoLimit1' : ( $data.userPhotoLimitMax <= 20 ? ' photoLimit2' : ( $data.userPhotoLimitMax <= 75 ? ' photoLimit3' : ''))) : '') ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Слишком много логики в шаблоне, может быть перенести в функцию в модуль?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Перенес логику
Делал ее для большей наглядности. Давайте уберу. |
Добавил функциональность по этой задаче #677
Добавденная функциональность (все нижеописанное только для модераторов и админов и не влияет на обычных пользователей):
Скрины (В системе 2 пользователя: админ с принудительным лимитом 50 и пользователь с лимитом 2 фото)
ПС. Не очень понял где настраивается перевод. Буду рад если покажите или исправите.
ПС. Нет возможности проверить насколько сильно агрегативные функции нагружают базу. Так же буду рад если есть идеи с оптимизацией запросов