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

FIx Finding Housekeeping Candidates and Modify Housekeeping Structure #749

Merged
merged 25 commits into from
Jan 11, 2024

Conversation

devleejb
Copy link
Member

@devleejb devleejb commented Jan 5, 2024

What this PR does / why we need it:

  • Refactored the FindDeactivateCandidates function, moving it from the Database Layer to the Service Layer
    • Due to the change in the function's layer, the Housekeeping-related tests from the Database Layer have been relocated to Integration Tests
  • Changed function name from listProjectInfos to FindNextNCyclingProjectInfos
  • Changed the implementation of the FindNextNCyclingProjectInfos function to retrieve a cyclic list of N projects with IDs greater than lastProjectID
  • Fix FindDeactivateCandidatesPerProject for memdb to return candidates of only one project

Which issue(s) this PR fixes:

Fixes # #744

Special notes for your reviewer:

  • Considering whether initializing the database in the Integration Test is the right approach.
  • Concerns arise due to the absence of clear code for each Unit Test, potentially leading to data dependency issues.
  • There is a remaining function FindDeactivateCandidatesPerProject in the Database Layer. Seeking opinions on whether it would be appropriate to rename it to something unrelated to Housekeeping.

Does this PR introduce a user-facing change?:

NONE

Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Attention: 33 lines in your changes are missing coverage. Please review.

Comparison is base (3b712df) 49.30% compared to head (6ad2ca3) 49.35%.
Report is 3 commits behind head on main.

Files Patch % Lines
server/backend/housekeeping/housekeeping.go 0.00% 20 Missing ⚠️
server/backend/database/memory/database.go 66.66% 4 Missing and 3 partials ⚠️
server/backend/database/mongo/client.go 64.70% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #749      +/-   ##
==========================================
+ Coverage   49.30%   49.35%   +0.04%     
==========================================
  Files          69       69              
  Lines       10094    10113      +19     
==========================================
+ Hits         4977     4991      +14     
- Misses       4601     4606       +5     
  Partials      516      516              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hackerwins hackerwins requested a review from krapie January 5, 2024 10:08
Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

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

I'm curious if there are ways to leverage the features of the Go language to further improve the code in a cleaner way.

I can't think of anything beyond what you've written, other than using a for statement to
simply code (maybe?) or refactoring redundant querying codes. I didn't find any syntax for circular queries in MongoDB either.

We can temporary apply this changes to fix urgent error, and then consider housekeeping service structure to improve overall housekeeping structure & performance.

Any ideas? @hackerwins

server/backend/database/mongo/client.go Outdated Show resolved Hide resolved
server/backend/database/memory/database.go Show resolved Hide resolved
server/backend/database/memory/database.go Outdated Show resolved Hide resolved
@krapie krapie force-pushed the housekeeping-on-single-project branch from c06536d to ce1ad89 Compare January 7, 2024 18:32
@devleejb
Copy link
Member Author

devleejb commented Jan 8, 2024

@krapie

I can't think of anything beyond what you've written, other than using a for statement to simply code (maybe?) or refactoring redundant querying codes. I didn't find any syntax for circular queries in MongoDB either.

I'm unfamiliar with Go lang, and I had concerns about whether the code could be simplified using techniques specific to the language. Thank you.

@devleejb devleejb requested a review from krapie January 8, 2024 02:03
@devleejb devleejb marked this pull request as draft January 8, 2024 06:19
Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

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

Can you request review after you open this PR? It seems like this is still in progress.

@devleejb devleejb marked this pull request as ready for review January 9, 2024 05:06
@devleejb devleejb requested a review from krapie January 9, 2024 05:06
Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

Refactored housekeeping service structure & test looks better than before 👍🏼
I have requested few convention-related changes.

server/backend/database/database.go Outdated Show resolved Hide resolved
server/backend/database/database.go Outdated Show resolved Hide resolved
server/backend/database/testcases/testcases.go Outdated Show resolved Hide resolved
server/backend/database/testcases/testcases.go Outdated Show resolved Hide resolved
server/backend/database/testcases/testcases.go Outdated Show resolved Hide resolved
test/integration/housekeeping_test.go Outdated Show resolved Hide resolved
test/integration/housekeeping_test.go Outdated Show resolved Hide resolved
test/integration/housekeeping_test.go Outdated Show resolved Hide resolved
test/integration/housekeeping_test.go Show resolved Hide resolved
@devleejb devleejb requested a review from krapie January 10, 2024 02:25
Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

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

LGTM.

@krapie
Copy link
Member

krapie commented Jan 11, 2024

@devleejb
Leaving few more comments on your notes:

Considering whether initializing the database in the Integration Test is the right approach.

initializing on top of the test function looks better than rentention_test.go.

Concerns arise due to the absence of clear code for each Unit Test, potentially leading to data dependency issues.

RunFindNextNCyclingProjectInfosTest() and RunFindDeactivateCandidatesPerProjectTest() test codes in testcases.go will do its job for now.

There is a remaining function FindDeactivateCandidatesPerProject in the Database Layer. Seeking opinions on whether it would be appropriate to rename it to something unrelated to Housekeeping.

This function can be only used for housekeeping purposes, and cannot normalize more. So I think name FindDeactivateCandidatesPerProject() is appropriate.

@krapie krapie force-pushed the housekeeping-on-single-project branch from d118639 to 6ad2ca3 Compare January 11, 2024 06:17
@krapie krapie changed the title Housekeeping not working when searching for a page that aligns exactly with the end of the entire project FIx Finding Housekeeping Candidates and Modify Housekeeping Structure Jan 11, 2024
@krapie krapie merged commit 145d4f8 into main Jan 11, 2024
3 checks passed
@krapie krapie deleted the housekeeping-on-single-project branch January 11, 2024 06:33
@krapie krapie added the bug 🐞 Something isn't working label Jan 11, 2024
@krapie krapie linked an issue Jan 11, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Housekeeping is Not Working Properly on Single Project
2 participants