Guess Country #563
Closed
Guess Country #563
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Generated by 🚫 Danger |
php-coder
requested changes
Apr 20, 2017
| } | ||
|
|
There was a problem hiding this comment.
Убери этот лишний перевод строки.
| @@ -35,4 +36,6 @@ | |||
| List<Object[]> getStatisticsOf(Integer collectionId, String lang); | |||
| List<LinkEntityDto> findAllAsLinkEntities(String lang); | |||
| LinkEntityDto findOneAsLinkEntity(String slug, String lang); | |||
|
|
|||
| String.class | ||
| ); | ||
|
|
||
| } catch (EmptyResultDataAccessException ex) { |
src/main/javascript/series/add.js
Outdated
|
|
||
| $.get(suggestCountryUrl, function (slug) { | ||
| if (slug == "") | ||
| return; |
src/main/javascript/series/add.js
Outdated
| @@ -21,4 +21,23 @@ function initPage() { | |||
| $('.js-with-tooltip').tooltip({ | |||
| 'placement': 'right' | |||
| }); | |||
|
|
|||
| if (suggestCountryUrl == null) { | |||
| return; | |||
There was a problem hiding this comment.
Вместо того, чтобы делать return в этом месте, давай изменим условие и внесем .get() внутрь условия:
if (suggestCountryUrl != null) {
$.get() {
...
}
}Позже, мы добавим еще один блок для категории.
| @@ -76,6 +76,12 @@ | |||
|
|
|||
| @Value("${country.find_country_link_info_by_slug}") | |||
| private String findCountryLinkEntityBySlugSql; | |||
|
|
|||
| @Value("${country.find_last_country_by_id}") | |||
| private String findLastCountryByIdSql; | |||
There was a problem hiding this comment.
findLastCountryByIdSql -> findLastCreatedByUserSql
| private String findLastCountryByIdSql; | ||
|
|
||
| @Value("${country.find_popular_country}") | ||
| private String findPopularCountrySql; |
There was a problem hiding this comment.
findPopularCountrySql -> findPopularCountryInCollectionSql
|
|
||
|
|
||
| @Override | ||
| public String suggestCountryForUser(Integer userId) { |
There was a problem hiding this comment.
Этот метод нужно переименовать в findLastCreatedByUser()
There was a problem hiding this comment.
P.S. И не забудь добавить автора.
| findPopularCountrySql, | ||
| Collections.<String, Object>emptyMap(), | ||
| String.class | ||
| ); |
There was a problem hiding this comment.
Все что внутри первого catch-а нужно вынести в отдельный метод findPopularCountryInCollection()
| @Override | ||
| @Transactional(readOnly = true) | ||
| @PreAuthorize(HasAuthority.CREATE_SERIES) | ||
| public String suggestCountryForUser(Integer userId) { |
There was a problem hiding this comment.
Этот метод должен вызывать сначала методы дао -- сначала findLastCreatedByUser(), а потом findPopularCountryInCollection().
Собственно, в сервисах должна быть вся логика, а дао методы должны быть как атомарные действия и следовать SRP (https://ru.wikipedia.org/wiki/Принцип_единственной_ответственности).
|
> + public String suggestCountryForUser(Integer userId) {
+
+ try {
+ return jdbcTemplate.queryForObject(
+ findLastCountryByIdSql,
+ Collections.singletonMap("created_by", userId),
+ String.class);
+ } catch (EmptyResultDataAccessException ignored) {
+ try {
+ return jdbcTemplate.queryForObject(
+ findPopularCountrySql,
+ Collections.<String, Object>emptyMap(),
+ String.class
+ );
+
+ } catch (EmptyResultDataAccessException ex) {
Переименуй ex в ignored.
Нельзя переименовать так-как ignored уже объявлена
2017-04-22 2:10 GMT+05:00 Vyacheslav Semushin <notifications@github.com>:
… ***@***.**** commented on this pull request.
------------------------------
In src/main/java/ru/mystamps/web/service/CountryServiceImpl.java
<#563 (comment)>:
> @@ -162,5 +163,16 @@ public long countUntranslatedNamesSince(Date date) {
return countryDao.getStatisticsOf(collectionId, lang);
}
-
+
+ /**
+ * @author Shkarin John
+ */
+ @OverRide
+ @transactional(readOnly = true)
+ @PreAuthorize(HasAuthority.CREATE_SERIES)
+ public String suggestCountryForUser(Integer userId) {
+ Validate.isTrue(userId != null, "User id must be non null");
+
+ return countryDao.suggestCountryForUser(userId);
P.S. На самом деле будет даже два вызова -- один когда нашли подсказку в
недавно созданной стране ("Country {} has been suggested to user #{} as a
recently created"), а другой когда используем популярную страну из
коллекции ("Country {} has been suggested to user #{} as popular in his
collection").
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#563 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AVJDARZ_hI_jHbcNMvCJaV2BMuoGR2Yeks5ryRthgaJpZM4NDgJ5>
.
|
|
In src/main/javascript/series/add.js
<#563 (comment)>:
> @@ -21,4 +21,23 @@ function initPage() {
$('.js-with-tooltip').tooltip({
'placement': 'right'
});
+
+ if (suggestCountryUrl == null) {
+ return;
+ }
+
+ $.get(suggestCountryUrl, function (slug) {
+ if (slug == "")
+ return;
+
+ var country = $("#guess_country");
#guess_country это <small> тег. Почему мы на него навешиваем on click
событие? Только чтобы заодно весь его скрыть?
идею не понял, а как без события клик? мне нужно скрыть эту ссылку и
подставить значение
2017-04-22 23:26 GMT+05:00 Евгений Шкарин <shkarin.john@gmail.com>:
… > + public String suggestCountryForUser(Integer userId) {
> +
> + try {
> + return jdbcTemplate.queryForObject(
> + findLastCountryByIdSql,
> + Collections.singletonMap("created_by", userId),
> + String.class);
> + } catch (EmptyResultDataAccessException ignored) {
> + try {
> + return jdbcTemplate.queryForObject(
> + findPopularCountrySql,
> + Collections.<String, Object>emptyMap(),
> + String.class
> + );
> +
> + } catch (EmptyResultDataAccessException ex) {
> Переименуй ex в ignored.
Нельзя переименовать так-как ignored уже объявлена
2017-04-22 2:10 GMT+05:00 Vyacheslav Semushin ***@***.***>:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/main/java/ru/mystamps/web/service/CountryServiceImpl.java
> <#563 (comment)>:
>
> > @@ -162,5 +163,16 @@ public long countUntranslatedNamesSince(Date date) {
>
> return countryDao.getStatisticsOf(collectionId, lang);
> }
> -
> +
> + /**
> + * @author Shkarin John
> + */
> + @OverRide
> + @transactional(readOnly = true)
> + @PreAuthorize(HasAuthority.CREATE_SERIES)
> + public String suggestCountryForUser(Integer userId) {
> + Validate.isTrue(userId != null, "User id must be non null");
> +
> + return countryDao.suggestCountryForUser(userId);
>
> P.S. На самом деле будет даже два вызова -- один когда нашли подсказку в
> недавно созданной стране ("Country {} has been suggested to user #{} as
> a recently created"), а другой когда используем популярную страну из
> коллекции ("Country {} has been suggested to user #{} as popular in his
> collection").
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#563 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AVJDARZ_hI_jHbcNMvCJaV2BMuoGR2Yeks5ryRthgaJpZM4NDgJ5>
> .
>
|
Yes, but later this code will be in a separate method, so don't forget to rename it there. |
I mean that why we have "on click" handler on |
|
I mean that why we have "on click" handler on <small> tag? Why it's not
on the <a> tag? I'm trying to understand why we're doing so in this way.
Потому что ссылка с тегом small, становится меньше и отсюда смотрится
лучше, на мой взгляд, ее осталось выровнять по центру и будем идеально)
23 апреля 2017 г., 0:25 пользователь Vyacheslav Semushin <
notifications@github.com> написал:
… #guess_country это тег. Почему мы на него навешиваем on click
событие? Только чтобы заодно весь его скрыть?
идею не понял, а как без события клик? мне нужно скрыть эту ссылку и
подставить значение
I mean that why we have "on click" handler on <small> tag? Why it's not
on the <a> tag? I'm trying to understand why we're doing so in this way.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#563 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AVJDAa_xu_-MZp-gPML4WxggGUKLRJT-ks5rylRDgaJpZM4NDgJ5>
.
|
|
блин я не так тебя понял( можно и на тег <a> сделать клик
23 апреля 2017 г., 12:27 пользователь Евгений Шкарин <shkarin.john@gmail.com
… написал:
I mean that why we have "on click" handler on <small> tag? Why it's not
> on the <a> tag? I'm trying to understand why we're doing so in this way.
Потому что ссылка с тегом small, становится меньше и отсюда смотрится
лучше, на мой взгляд, ее осталось выровнять по центру и будем идеально)
23 апреля 2017 г., 0:25 пользователь Vyacheslav Semushin <
***@***.***> написал:
#guess_country это тег. Почему мы на него навешиваем on click
> событие? Только чтобы заодно весь его скрыть?
>
> идею не понял, а как без события клик? мне нужно скрыть эту ссылку и
> подставить значение
>
> I mean that why we have "on click" handler on <small> tag? Why it's not
> on the <a> tag? I'm trying to understand why we're doing so in this way.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#563 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AVJDAa_xu_-MZp-gPML4WxggGUKLRJT-ks5rylRDgaJpZM4NDgJ5>
> .
>
|
|
+country.find_last_country_by_id = \
+ SELECT c.slug \
+ FROM series s \
+LEFT JOIN countries c ON c.id = s.country_id \
+ WHERE s.created_by = :created_by \
+ ORDER BY s.created_at DESC LIMIT 1
+
+country.find_popular_country = \
+ SELECT c.slug \
+ FROM series s \
+LEFT JOIN countries c ON c.id = s.country_id \
+ GROUP BY country_id \
+ ORDER BY COUNT(*) DESC LIMIT 1
Этот запрос делает не то, что было в задании: "most (or all) series in
user's collection are in country X". Мы должны искать популярную страну в
коллекции пользователя, а не по всему сайту.
Я же правильно понимаю, что искать популярную страну в коллекции
пользователя нужно в таблице series? Если это так то
country.find_popular_country - никогда не выполниться, если в таблице есть
хоть одна запись с этим пользователем.
23 апреля 2017 г., 12:29 пользователь Евгений Шкарин <shkarin.john@gmail.com
… написал:
блин я не так тебя понял( можно и на тег <a> сделать клик
23 апреля 2017 г., 12:27 пользователь Евгений Шкарин <
***@***.***> написал:
I mean that why we have "on click" handler on <small> tag? Why it's not
>> on the <a> tag? I'm trying to understand why we're doing so in this way.
>
>
> Потому что ссылка с тегом small, становится меньше и отсюда смотрится
> лучше, на мой взгляд, ее осталось выровнять по центру и будем идеально)
>
> 23 апреля 2017 г., 0:25 пользователь Vyacheslav Semushin <
> ***@***.***> написал:
>
> #guess_country это тег. Почему мы на него навешиваем on click
>> событие? Только чтобы заодно весь его скрыть?
>>
>> идею не понял, а как без события клик? мне нужно скрыть эту ссылку и
>> подставить значение
>>
>> I mean that why we have "on click" handler on <small> tag? Why it's not
>> on the <a> tag? I'm trying to understand why we're doing so in this way.
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <#563 (comment)>,
>> or mute the thread
>> <https://github.com/notifications/unsubscribe-auth/AVJDAa_xu_-MZp-gPML4WxggGUKLRJT-ks5rylRDgaJpZM4NDgJ5>
>> .
>>
>
>
|
Если можно, то давай так и сделаем. Это выглядит более логично. |
В series находятся все серии, которые доступны на сайте. Тебе нужны таблицы collections (которая связывает пользователя с его коллекцией) и collections_series (которая связывает коллекцию с сериями, которые в нее входят). |
Кстати, да, не забудь выровнять ее. |
php-coder
reviewed
Apr 23, 2017
| JOIN countries c ON c.id = s.country_id \ | ||
| WHERE s.created_by = :created_by \ | ||
| ORDER BY s.created_at DESC\ | ||
| LIMIT 1 |
There was a problem hiding this comment.
Отформатируй, пожалуйста, чтобы ключевые слова были справа, а остальное слева и посередине был столбец из пробелов:
- перед JOIN-ом и LIMIT-ом нужно добавить по пробелу
- после DESC нужно добавить пробел перед переводом строки
|
Осталось немного:
Вроде бы все. |
|
Доделаю, просто не было времени что заняться задачей
24 апреля 2017 г., 0:36 пользователь Vyacheslav Semushin <
notifications@github.com> написал:
… Осталось немного:
- исправить SQL-запрос (и соответствующий метод дао)
- немного отформатировать другой SQL-запрос
- отцентрировать ссылку
- переместить идентификатор с тега small на a
- исправить ошибки от checkstyle/pmd:
- Добавить @SuppressWarnings для Avoid excessively long variable
names like findPopularCountryInCollectionSql
- разбить строчки с логгированием на 5 строк
Вроде бы все.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#563 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AVJDAXX5nWNb5v96Yrp3dXN3zoz-uN-yks5ry6hOgaJpZM4NDgJ5>
.
|
|
- разбить строчки с логгированием на 5 строк
это где?
ты задачу с
#547 (comment) еще
не поставил? если нет я поставлю
Вот с выравнивание по центру ссылки, я еще подумаю, конечно, что у меня
когда пытался это сделать не получалось, стиль сбрасывался, наверное нужно
th добавить
26 апреля 2017 г., 13:36 пользователь Евгений Шкарин <shkarin.john@gmail.com
… написал:
Доделаю, просто не было времени что заняться задачей
24 апреля 2017 г., 0:36 пользователь Vyacheslav Semushin <
***@***.***> написал:
Осталось немного:
>
> - исправить SQL-запрос (и соответствующий метод дао)
> - немного отформатировать другой SQL-запрос
> - отцентрировать ссылку
> - переместить идентификатор с тега small на a
> - исправить ошибки от checkstyle/pmd:
> - Добавить @SuppressWarnings для Avoid excessively long variable
> names like findPopularCountryInCollectionSql
> - разбить строчки с логгированием на 5 строк
>
> Вроде бы все.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#563 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AVJDAXX5nWNb5v96Yrp3dXN3zoz-uN-yks5ry6hOgaJpZM4NDgJ5>
> .
>
|
Это предупреждения на длинные строки тут #563 (comment)
Нет. Создай, пожалуйста. |
|
Привет, у меня тут сложности с запросом
возникли (country.find_popular_country_from_user_collection ), я правильно
понимаю идею:
1) По user_id из collections мы получаем collections.id
2) Затем мы делаем выборку в collections_series по collections.id,
группируем и находим популярную series.id
3) А как нам slug страны получить? искать в таблице series, country.id по
series.id и потом обращаться в countries за slug?
и все это нужно одним запросом сделать?
2017-04-30 0:12 GMT+05:00 Coveralls <notifications@github.com>:
… [image: Coverage Status] <https://coveralls.io/builds/11304471>
Coverage decreased (-0.8%) to 80.353% when pulling *a5edb40
<a5edb40>
on Shkarin:gh571_prediction_country* into *0abdb81
<0abdb81>
on php-coder:master*.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#563 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AVJDAboj0b-ZiJcp6vqdnGQwzwlaK7NYks5r04ubgaJpZM4NDgJ5>
.
|
Shkarin
force-pushed
the
gh571_prediction_country
branch
from
7ad85a9 to
90ab13d
Compare
|
Changes Unknown when pulling 90ab13d on Shkarin:gh571_prediction_country into ** on php-coder:master**. |
|
Привет,
Нужно заджойнить все таблицы - collections, collections_series, series,
countries; отобрать коллекцию по владельцу. После этого сгиуппировать по
countries.id и взять самую популярную.
суббота, 29 апреля 2017 г. пользователь John написал:
… Привет, у меня тут сложности с запросом
возникли (country.find_popular_country_from_user_collection ), я
правильно
понимаю идею:
1) По user_id из collections мы получаем collections.id
2) Затем мы делаем выборку в collections_series по collections.id,
группируем и находим популярную series.id
3) А как нам slug страны получить? искать в таблице series, country.id по
series.id и потом обращаться в countries за slug?
и все это нужно одним запросом сделать?
--
Slava Semushin
|
|
Changes Unknown when pulling c6ba408 on Shkarin:gh571_prediction_country into ** on php-coder:master**. |
Shkarin
force-pushed
the
gh571_prediction_country
branch
from
c6ba408 to
d881843
Compare
|
Changes Unknown when pulling d881843 on Shkarin:gh571_prediction_country into ** on php-coder:master**. |
|
@Shkarin Всё, можно ревьювить или ты еще планируешь что-то доделать? |
|
Можно, но я еще не до конца проверил правильность работы
2017-05-04 1:25 GMT+05:00 Vyacheslav Semushin <notifications@github.com>:
… @Shkarin <https://github.com/Shkarin> Всё, можно ревьювить или ты еще
планируешь что-то доделать?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#563 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AVJDATlNXVZHqYc-C_t26zXQvz7tF8AEks5r2OLIgaJpZM4NDgJ5>
.
|
|
Привет, вроде все работает
4 мая 2017 г., 10:12 пользователь Евгений Шкарин <shkarin.john@gmail.com>
написал:
… Можно, но я еще не до конца проверил правильность работы
2017-05-04 1:25 GMT+05:00 Vyacheslav Semushin ***@***.***>:
> @Shkarin <https://github.com/Shkarin> Всё, можно ревьювить или ты еще
> планируешь что-то доделать?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#563 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AVJDATlNXVZHqYc-C_t26zXQvz7tF8AEks5r2OLIgaJpZM4NDgJ5>
> .
>
|
|
Merged in f96d776 commit. |
|
Спасибо! Я добавил еще несколько мелких улучшений (можешь глянуть 7 дополнительных коммитов). Чуть позже я создам еще несколько задач, часть из которых нужно будет сделать до того как будут реализованы подсказки для категорий. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addressed to #517
Supersede #547 (that php-coder closed accidentally)
Check before merge:
After merge: