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

Take the send history into account in the transportState filter #90

Conversation

flkasper
Copy link
Contributor

@flkasper flkasper commented Sep 30, 2024

  • Add filter on frosh-mail-archive-index.twig
  • add/fix of router-link on createdAt column in frosh-mail-archive-index.twig
  • add missing positionIdentifier to card in frosh-mail-resend-history.html.twig
  • Refactoring of sourceMail/sourceMails to parent/children

Follow up feature from PR #84

@flkasper flkasper force-pushed the feature/filter-last-history-mail-transport-state branch from a1ead91 to 6961ea8 Compare October 1, 2024 07:43
@@ -47,6 +56,7 @@ protected function defineFields(): FieldCollection
(new LongTextField('eml', 'eml'))->addFlags(new AllowHtml()),
(new StringField('eml_path', 'emlPath', 2048)),
(new StringField('transport_state', 'transportState'))->addFlags(new Required()),
new BoolField('history_last_mail', 'historyLastMail'),
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like the idea of adding a separate column for historyLastMail. Why not just do a groupBy on sourceMailId/parentId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schneider-felix
As discussed on Saturday, I tried a GroupBy via a historyGroupId column.
This column can be realized as a virtual column that reflects the value of parent_id/id.

However, this does not work as expected.
When I use criteria.addGrouping('historyGroupId') in the admin, the result is not correct.
The last element is not returned as expected.

Did I use the addGrouping incorrectly or does it not work like a GroupBy in SQL?
The customized code can be found at: Link to Branch

If we cannot fix the grouping, we will have to stick with the less attractive solution with historyLastMail.

Copy link
Member

Choose a reason for hiding this comment

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

@flkasper thanks for the update. I'll have a look.

+ add/fix of router-link on createdAt column in frosh-mail-archive-index.twig
+ add missing positionIdentifier to card in frosh-mail-resend-history.html.twig
@flkasper flkasper force-pushed the feature/filter-last-history-mail-transport-state branch from 6961ea8 to c0ce059 Compare October 7, 2024 10:24
@schneider-felix
Copy link
Member

@flkasper this PR sparked a discussion about the way mails are stored in the database. We came to the conclusion that the table structure needs some work. I will be doing some major changes in the near future. We will most likely introduce an additional table to track "resends".

Therefore I will have to close this PR for the moment. Thank you very much for the time and effort.

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