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

feat.activities: dto mapping to endpoint #5911

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

aberonni
Copy link
Contributor

@aberonni aberonni commented Oct 9, 2024

AB#30664

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added tests wherever relevant
  • I have made sure that all automated checks pass before requesting a review
  • I do not need any deviation from our PR guidelines

@aberonni aberonni force-pushed the aberonni/activities-mapping branch from 6bd726a to 110d840 Compare October 9, 2024 15:01
@aberonni aberonni force-pushed the aberonni/activities-mapping branch from 110d840 to a1960c6 Compare October 9, 2024 15:05
@aberonni aberonni force-pushed the aberonni/activities-mapping branch from a1960c6 to 0e7834b Compare October 9, 2024 15:22
@aberonni aberonni force-pushed the aberonni/activities-mapping branch from 0e7834b to abdbc2e Compare October 10, 2024 14:08
@aberonni aberonni force-pushed the aberonni/activities-mapping branch from abdbc2e to 8fc0512 Compare October 10, 2024 14:13
@aberonni aberonni force-pushed the aberonni/activities-mapping branch from 8fc0512 to 977f51b Compare October 10, 2024 15:23
@aberonni aberonni marked this pull request as ready for review October 10, 2024 15:32
@aberonni aberonni force-pushed the feat.activities-endpoint branch from 774809b to 94ca99d Compare October 11, 2024 08:30
@aberonni aberonni force-pushed the aberonni/activities-mapping branch from 977f51b to 3e240af Compare October 11, 2024 08:33
@aberonni aberonni force-pushed the feat.activities-endpoint branch from 94ca99d to 836f386 Compare October 11, 2024 13:02
@aberonni aberonni force-pushed the aberonni/activities-mapping branch from 8de13d2 to 4889e59 Compare October 11, 2024 13:02
Copy link
Contributor

@diderikvw diderikvw left a comment

Choose a reason for hiding this comment

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

Now this is easy to read code! :)

@aberonni
Copy link
Contributor Author

aberonni commented Oct 15, 2024

@diderikvw I've tried to align with the guidelines to have one export per file, but I think this is a great example of why I didn't like this guideline to begin with (as per our old discussions about it).

I think that what I've pushed

image

makes it simpler to answer the question "where should I put this?" but IMHO it does not make the code simpler / easier-to-use / easier-to-read / easier-to-navigate than what was there before:

image

Not to mention that it is twice the amount of code:
image

Copy link
Contributor

@diderikvw diderikvw left a comment

Choose a reason for hiding this comment

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

Ok, great you addressed these.
Just a few small things left.

One of which here (because I cannot reply there for some reason):

I've tried to align with the guidelines to have one export per file, but I think this is a great example of why I didn't like this guideline to begin with (as per our old discussions about it).

I am happy to have a conversation about changing this convention. But I do want to have some convention on this and not let it depend on "whatever the developer favors who happens to work on this code". IMO having it "all the same way" does improve readability, even if TS code-wise it is not as efficient as could be.

export interface DataChangeActivity extends BaseActivity {
type: ActivityTypeEnum.DataChange;
attributes: {
fieldName: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the necessity of having the fields to be | null? Not having this would make using the interface simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This stems from the input data being nullable:

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I would like to discuss this with @PeterSmallenbroek , related to the ##TODOs already in the code.

services/121-service/src/activities/dtos/activities.dto.ts Outdated Show resolved Hide resolved
@diderikvw diderikvw merged commit 100ed72 into feat.activities-endpoint Oct 17, 2024
6 checks passed
@diderikvw diderikvw deleted the aberonni/activities-mapping branch October 17, 2024 07:11
aberonni added a commit that referenced this pull request Oct 17, 2024
* feat.activities: dto mapping to endpoint

AB#30664

* add permissions handling AB#30716

* One export per file
aberonni added a commit that referenced this pull request Oct 17, 2024
* feat.activities: dto mapping to endpoint

AB#30664

* add permissions handling AB#30716

* One export per file
PeterSmallenbroek pushed a commit that referenced this pull request Oct 18, 2024
* feat.activities: dto mapping to endpoint

AB#30664

* add permissions handling AB#30716

* One export per file
PeterSmallenbroek pushed a commit that referenced this pull request Oct 18, 2024
* feat.activities: dto mapping to endpoint

AB#30664

* add permissions handling AB#30716

* One export per file
aberonni added a commit that referenced this pull request Oct 18, 2024
* activity module & type

* refactor: event scoped repository

* repositories & types

* Activities review Peter's code (#5908)

Some comments and small changes

* activities: fix unit tests (#5914)

* activities: fix unit tests

* temporarily disable exports

* feat.activities: dto mapping to endpoint (#5911)

* feat.activities: dto mapping to endpoint

AB#30664

* add permissions handling AB#30716

* One export per file

* Resolve TODOs activities endpoint (#5928)

* resolve TODOs

* Update services/121-service/src/activities/activities.controller.ts

Co-authored-by: Domenico Gemoli <dgemoli@redcross.nl>

---------

Co-authored-by: Domenico Gemoli <dgemoli@redcross.nl>

---------

Co-authored-by: diderikvw <diderik@diderikvanwingerden.com>
Co-authored-by: Domenico Gemoli <dgemoli@redcross.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants