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

Add CardDetail serialization model #3782

Merged
merged 6 commits into from
May 6, 2022
Merged

Conversation

Raudius
Copy link
Contributor

@Raudius Raudius commented May 4, 2022

  • Target version: master

Summary

Adds CardDetail + BoardSummary models which centralises serialisation of Card database models. This will be useful for the upcoming changes in the overview page (#2807) which will require attaching additional information to the Card (namely: board name) as it will not need to be added in multiple parts of the code.

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Documentation (manuals or wiki) has been updated or is not required

@Raudius Raudius self-assigned this May 4, 2022
@Raudius Raudius changed the title Add CardDetail model Add CardDetail serialization model May 5, 2022
@Raudius Raudius requested review from a team, vinicius73 and max-nextcloud and removed request for a team May 5, 2022 14:55
@vinicius73
Copy link
Member

vinicius73 commented May 5, 2022

LGTM;

But, what do you think about renaming Models to Presenters or something similar @Raudius ?
We already have Db folder and we know models aren't directly related to database things.
But it can create some confusion in the future. What do you think?

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Nice, good approach I think to move the representation logic out of the actual Db entity layer. I'd be fine with the Model naming as well as its a common term here 👍

@juliusknorr
Copy link
Member

@Raudius Mind to squash the commits a bit (especially the different cs fix ones)?

@Raudius Raudius force-pushed the enh/backend_serialization_models branch from bffedf1 to ce0de07 Compare May 6, 2022 09:26
Signed-off-by: Raul <raul@nextcloud.com>
… model

Signed-off-by: Raul <raul@nextcloud.com>
Signed-off-by: Raul <raul@nextcloud.com>
Signed-off-by: Raul <raul@nextcloud.com>
Signed-off-by: Raul <raul@nextcloud.com>
@Raudius Raudius force-pushed the enh/backend_serialization_models branch from ce0de07 to 9fca104 Compare May 6, 2022 09:29
@Raudius
Copy link
Contributor Author

Raudius commented May 6, 2022

But, what do you think about renaming Models to Presenters or something similar @Raudius ?
@vinicius73

I copied the idea (and name) from the Collectives app. So I think I will leave it as Model for consistency, at least within the Office apps.

@Raudius Raudius merged commit b86fbb3 into master May 6, 2022
@delete-merged-branch delete-merged-branch bot deleted the enh/backend_serialization_models branch May 6, 2022 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants