-
Notifications
You must be signed in to change notification settings - Fork 12
Issue 6 timepad integration #94
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
Issue 6 timepad integration #94
Conversation
|
@AnatolyKulakov тебе тоже интересно будет |
|
Потому что Autofac надо выпилить, он остался от прототипа сервиса. C стандартным все ок |
|
Добавил работу с различными сообществами:
|
|
А давай наоборот структуру конфигурации: Мы планируем для каждого сообщества свой файл конфигурации - так будет проще собирать конфигурацию |
|
Настройки тогда для сообщества |
Это сложный вопрос. По хорошему, все необходимые данные должны быть в моделе и никаких аргументов не нужно. Но так-как ты делаешь первую интеграцию, мы обнаруживаем новые поля, которых в моделе нет. Нам нужно изменять серверную модель. Но не сильно, ибо данная задача не про это совсем. Если как-то возможно сделать быстрый хак и не отвлекаться на модель, то нужно это сделать. @egorikas должен лучше знать как сейчас устроена модель и как можно её подхачить, чтобы не сломать контракт с UI. Я вижу необходимость в следующих изменениях: а. Добавить в Meetup обязательное текстовое поле ShortDescription Сейчас (пока UI не осилили), для TimePad'а нужно сделать следующие defaults: а. Потом редактору эти параметры, при желании, можно будет поменять в конструкторе самого TimePad.
Да, в #93 это немного обсуждали.
На UI должны уходить DTO для UI. Наши интеграции должны работать с бизнесс моделью. Она должна быть другой, не такой как для UI. Иначе мы никогда не сможем её менять без того чтобы всё не разрушить. Бизнес модель должна мапиться в UI DTO.
Будь внимателен. Этого поля ещё нет в Аудите. Это проявление недержания UI'я. Поэтому не ясно откуда это поле берётся и как загружается для старых встреч.
Да, настройки должны храниться не по сервисам, а по сообществам (см. #93). В качестве идентификатора сообщества (для файлов и секций) надо использовать Community.Id. Ибо не понятно, что такое "Spb.cshtml" и как его сматчить с сообществом.
Не надо, это избыточно. Это всё есть в API. Смотри метод Get-TimePadOrganization() @rafaelldi , не пытайся догадаться как работает API. Заведи тестовую организацию на TimePad и попробуй посоздавать митапы там. Или лучше спроси в поддержке, кажется что у них есть такая тестовая организация, специально для экспериментов с API. |
Работа с API TimePadПообщался с поддержкой TimePad:
Работа с тенантами
где
У нас кстати нет номера встречи ( Остальное поправил. Потестировал, события создаются. Осталось доделать нормальный шаблон и можно смотреть. |
Ясно. Ну пока руками будем добавлять.
А можешь подробнее рассказать что такое
Кажется что тебе не нужен префикс "Community". Идентификатор сообщества уникален.
А давай сделаем такой путь:
Да. Наверное в Аудите его и не надо, ибо он почти всегда парсится. И ещё не все митапы обязаны иметь номер. Надо эту тему получше продумать, если мы начинаем на номер где-то полагаться. |
|
Черновики позволяют не указывать все обязательные настройки. Если событие опубликовано, все его обязательные параметры должны быть указаны. Так как мы сразу заполняем все нужные параметры, для нас различий между Я просто использовал префикс |
|
@rafaelldi А почему ты не использовал стандартный Options pattern? |
|
Я в методе расширения для |
|
@rafaelldi Рестарт ради нового сообщества это не большая проблема. Нужно только не забыть в документации отметить это требование. Переделай лучше любую работу с настройками через Options. Это избавит от кучи проблем. В том числе и от регулярок. А мы потом в эти опции соберём правильные значения из файла, из Compose Env, из Azure и прочих разных мест. |
А почему 'int'? С моей точки зрения, "ID сообщества" это строка "SpbDotNet", "MskDotNet", и т.п. |
|
Ну по модельке |
|
Не, не, не. Это UI модель. Она не подходит для бизнес-логики. |
|
Я пока сделал темплейт похожим на SpbDotNet события и переписал через
В остальном вроде всё готово, если не считать эти модели. Я их брал из |
|
@rafaelldi 1. Постер было бы идеально копировать с предыдущего митапа. |
|
|
@AnatolyKulakov тебя все устраивает с точки зрения заказчика? |
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <Content Include="Templates\1\TimePad.cshtml"> |
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.
Почему "1"? Кажется договаривались всё складывать в подпапку Community.Id. В данном случае можно всё делать для SpbDotNet, на нём будем обкатывать для остальных.
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.
Id сейчас int у модели, можно брать ExportId, там как раз текстовые названия как SpbDotNet лежат
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.
Да, когда я упоминал поля типа Community.Id я везде имелл ввиду схему Аудита. Не схему UI. То как это называется в UI здесь не важно.
| <br/> | ||
| Telegram: <a href="https://t.me/spbdotnet">https://t.me/SpbDotNet</a> | ||
| <br/> | ||
| Meetup.com: <a href="https://www.meetup.com/SpbDotNet/">https://www.meetup.com/SpbDotNet</a> |
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.
Ссылки желательно брать из Community
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.
@egorikas , а мы можем обновить модель на актуальную схему Аудита?
| { | ||
| _clientFactory = factory; | ||
| _optionsAccessor = options; | ||
| var rootDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().CodeBase); |
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.
Путь к рабочей директории лучше получать из AppDomain.BaseDirectory. Ибо текущая сборка может лежать где угодно (например в отдельной темповой папке с версией и изоляцией, как делает IIS). А все конфиги всегда будут в BaseDirectory.
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.
Поправил
| _clientFactory = factory; | ||
| _optionsAccessor = options; | ||
| var rootDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().CodeBase); | ||
| var appRoot = rootDir.Substring(5); |
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.
Не очень понятно что такое "5", откуда взялось, что обозначает и почему это должно работать?
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.
Там путь всегда возвращался в виде: file:/..., эти пять символов я удалял. Но сейчас в нормальном виде путь возвращается, так что убрал это
| var shortDescription = $"{dateDescription} {friendsDescription} состоится встреча {meetup.Community.Name}"; | ||
|
|
||
| var templateModel = CreateTemplateModel(meetup); | ||
| var htmlDescription = await _razorEngine.CompileRenderAsync($"Templates/{meetup.CommunityId}/TimePad.cshtml", templateModel); |
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.
Лучше использовать абсолютные пути (Path.Combine(BaseDirectory)). Тогда такой код будет более переносим между различными системами и будет работать более предсказуемо и стабильно. Ибо предсказать где в данный момент находится Environment.CurrentDirectory (которая неявно используется в данном случае) абсолютно невозможно.
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.
Не, там как базовый путь используется то, что указали в методе .UseFilesystemProject(), т.е. получается сейчас AppDomain.CurrentDomain.BaseDirectory
| { | ||
| Name = "Входной билет", | ||
| Price = 0, | ||
| Send_personal_links = true |
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.
Почему true? На что это влияет?
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.
Написано Категория билетов с отправкой персональных ссылок. У текущих событий стоит true, я с них брал
| if (friends is null || friends.Count == 0) | ||
| return string.Empty; | ||
|
|
||
| var description = "в гостях у " + (friends.Count == 1 ? "компании " : "компаний ") + |
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()), остальные просто спонсоры, их тут указывать не надо.
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.
Поправил
| } | ||
| } | ||
|
|
||
| public async Task PublishEventAsync(Meetup meetup, CancellationToken ct) |
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.
Надеюсь сейчас этот метод ниоткуда не вызывается? Первое время, пока не отладимся, анонсы будут публиковаться только вручную с сайта TimePad. У нас пока слишком много необходимых ручных правок нужно сделать перед автоматической публикацией
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.
Ты же добавил контроллер. Там они используются. Вот я бы Publish от греха подальше убрал бы из контроллера, чтобы случайно его не вызвать
|
|
||
| var communityOrganization = introspect.Organizations?.SingleOrDefault(o => o.Name.Contains(community.Name)); | ||
| if (communityOrganization is null) | ||
| throw new Exception("Can't find community TimePad organization"); |
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.
А у нас нормально исключения на UI отображаются? Я, как пользователь портала, увижу причину ошибки?
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.
Стандартного отлова исключений через UseExceptionHandler я вроде не нашёл, а в остальном не знаю
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.
@AnatolyKulakov сейчас универсально вроде не ловится нигде. Я по крайней мере не писал общего Filter для перехвата
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.
У нас не ловится и ты все увидишь. Логи тоже надо добавить #101
| SpeakersDescriptions = s.Talk.Speakers.Select(sp => new TemplateModel.TemplateSpeaker | ||
| { | ||
| Description = sp.Speaker.Description, | ||
| AvatarUrl = $"https://raw.githubusercontent.com/DotNetRu/Audit/master/db/speakers/{sp.Speaker.ExportId}/avatar.small.jpg" |
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.
Думаю надо всё-таки создать issue по загрузки фотографий на их стандартное хранилище. Очень не хочется привязывать структуру хранения в Аудите к TimePad'у. Поменяем место хранения фотографий и старые анонсы превратятся в тыкву. А для кого-то они могут быть важны.
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.
Создал ишью #100
|
@egorikas , да отличная работа. А как это сейчас будет работать? Похоже что никак? Мы можем прикрутить какую-нибудь кнопочку чтобы можно было тестировать? Можно даже через свагер: чтобы вбить |
|
Ещё надо не забыть регистрировать конфигурацию с токеном, потому что сервис ожидает |
|
@AnatolyKulakov мы можем метод вывалить в сваггер и им можно будет пользоваться. нужный умел фронтендер, чтобы поддержать на UI |
|
@rafaelldi @AnatolyKulakov давайте реально сделаем контроллер, чтобы можно было кнопочку в сваггере потыкать. мне нравится идея, для тестов, то что нужно |
|
Добавил контроллер |
|
@AnatolyKulakov вродь как MVP все ок |
|
@egorikas , да шикарно, давай вливать и дальше допиливать |
| var events = await GetLastPrivateEvents(communityOrganization.Id, timePadClient, ct); | ||
| var meetupEvent = events.FirstOrDefault(e => e.Name == meetup.Name); | ||
| if (meetupEvent is null) | ||
| throw new Exception("Can't find meetup event"); |
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.
По хорошему, надо завести специальные еxception, чтобы потом не было проблем с их определением.
Issue #6
Сделал базовый прототип, чтобы дальше детальнее обсуждать (делал пока без тенантов). Подскажите, пожалуйста, по этим вопросам.
Вопросы о том, что откуда брать:
IdиSubdomainорганизации на таймпаде, откуда лучше мне их получать?Starts_atиEnds_at- не нашёл временных рамок для всего митапа, есть только для отдельных сессий. Можно выбирать по ним максимальное и минимальное время;Description_short- тоже не нашёл описаний;City- у нас это enum, видимо, стоит простой маппер написать на названия городов;Tickets_limit- тоже нет.Какие-то из этих параметров можно получать не из объекта
Meetup, а из дополнительных параметров метода, какие лучше?Вопросы, связанные с api:
idсобытия на таймпаде мы хранить не будем, когда нужно будет обновлять, то надо смотреть по совпадению имени?custom: Объект с дополнительными полями, специфичными для данной организации, который вроде можно использовать, хотя не понятно, как он вообще выглядит, плюс ещё надо фоточку партнера вставлять;draft. Если что буду в сапорт обращаться.Регистрацию в DI я через
IServiceCollectionсделал, потому что пока не разбирался, как Autofac добавить. У меня вопрос скорее из любопытства, а почему не стали использовать стандартный DI?