Skip to content
This repository has been archived by the owner on Aug 11, 2023. It is now read-only.

feat: Forums CRUD #27

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

feat: Forums CRUD #27

wants to merge 19 commits into from

Conversation

tan-yun-e
Copy link
Contributor

No description provided.

@tan-yun-e tan-yun-e changed the title Forums CRUD feat: Forums CRUD Jul 1, 2023
@codecov
Copy link

codecov bot commented Jul 1, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@3c832c0). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #27   +/-   ##
=======================================
  Coverage        ?   15.78%           
=======================================
  Files           ?       10           
  Lines           ?      817           
  Branches        ?        0           
=======================================
  Hits            ?      129           
  Misses          ?      678           
  Partials        ?       10           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tan-yun-e tan-yun-e requested a review from qin-guan July 1, 2023 05:06
Copy link
Member

@qin-guan qin-guan left a comment

Choose a reason for hiding this comment

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

I am thinking whether it would be good to merge this together into the group domain, instead of having a folder inside group.

In retrospect, seems like it was a bad idea to include httphandler inside the domain folders (grp, inst, etc). They should be outside since the REST api structure would be different from our domain (use case) methods

internal/entity/forum/forum.go Outdated Show resolved Hide resolved
internal/group/forum/error.go Outdated Show resolved Hide resolved
internal/payload/forum.go Outdated Show resolved Hide resolved
internal/group/forum/usecase.go Outdated Show resolved Hide resolved
internal/group/forum/usecase.go Outdated Show resolved Hide resolved
internal/group/forum/usecase.go Outdated Show resolved Hide resolved
internal/group/forum/usecase.go Outdated Show resolved Hide resolved
internal/group/forum/usecase.go Outdated Show resolved Hide resolved
internal/group/forum/usecase.go Outdated Show resolved Hide resolved
internal/group/forum/usecase.go Show resolved Hide resolved
@tan-yun-e
Copy link
Contributor Author

I am thinking whether it would be good to merge this together into the group domain, instead of having a folder inside group.

I prefer the small amount of organisation having it in a separate folder gives

In retrospect, seems like it was a bad idea to include httphandler inside the domain folders (grp, inst, etc). They should be outside since the REST api structure would be different from our domain (use case) methods

Do I create a separate folder for httphandler for forum group and institution

Copy link
Contributor Author

@tan-yun-e tan-yun-e Jul 1, 2023

Choose a reason for hiding this comment

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

WIP did not mean to stage files on forumpost

@qin-guan
Copy link
Member

qin-guan commented Jul 1, 2023

I prefer the small amount of organisation having it in a separate folder gives

In that case, better to shift outside of the group folder.

Do I create a separate folder for httphandler for forum group and institution

No, I'll work on it later

internal/payload/forum.go Outdated Show resolved Hide resolved
@qin-guan
Copy link
Member

qin-guan commented Jul 5, 2023

@tan-yun-e Do you have any changes left planned?

@tan-yun-e
Copy link
Contributor Author

@tan-yun-e Do you have any changes left planned?

Are there changes still needed

@qin-guan qin-guan mentioned this pull request Jul 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants