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

ListThread service: add is_mine parameter #943

Merged
merged 5 commits into from
Apr 24, 2023
Merged

Conversation

GeoffreyHuck
Copy link
Contributor

@GeoffreyHuck GeoffreyHuck commented Apr 11, 2023

Issue #888

This PR handles the is_mine parameter for the listThread service.

Some rights between this and the getThread have been factorized into WhereUserCanHelp:

	// the following rules all matches:
	// the current-user is descendant of the thread helper_group
	// the thread is either open (=waiting_for_participant or =waiting_for_trainer), or closed for less than 2 weeks
	// the current-user has validated the item

The function NewThreadStore has been added because when we use a function defined on *DB, we end up being unable to use functions defined on *ThreadStore anymore, because we get a *DB in return. The reason relates to types: A *ThreadStore is a *DataStore is a *DB, but a *DB is not a *ThreadStore.

I'm not sure what's the best thing to do here, maybe there exists a pattern that would handle this case smoothly in go. The code seems to struggle with this everywhere, where the pattern used is:

func (s *ItemAncestorStore) DescendantsOf(ancestorID int64) *ItemItemStore {
	return &ItemItemStore{NewDataStoreWithTable(
		s.Where("items_ancestors.ancestor_item_id = ?", ancestorID), s.tableName,
	)}
}

At other times, it returns a *DB instead of a DataStore to avoid this issue. But then we lose consistence with some methods defined on a specific store which returns a specific *DataStore (eg. *ThreadStore, *GroupStore, ...), and others who return a *DB. It would be nice to decide which pattern to use for uniformization, and if possible for simplicity.

Note: I would have liked to add a "default" to the switch statement in list_threads to guarantee the correctness of the parameters. But the parameters are checked before in a function, and it made the code within the default unreachable, so it didn't pass the code coverage tests.

@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

Merging #943 (07bf7f1) into master (26ffdc1) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #943   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          225       225           
  Lines        13466     13503   +37     
=========================================
+ Hits         13466     13503   +37     
Impacted Files Coverage Δ
app/api/threads/list_threads.go 100.00% <100.00%> (ø)
app/database/thread_store.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@smadbe
Copy link
Contributor

smadbe commented Apr 12, 2023

Missing ref to the issue it is fixing.

@GeoffreyHuck
Copy link
Contributor Author

Missing ref to the issue it is fixing.

It's not finished yet, still requires the parameter item_id and the sorting/filter/pagination.

Base automatically changed from getThreads2 to master April 19, 2023 06:42
@GeoffreyHuck GeoffreyHuck force-pushed the getThreadsIsMine branch 2 times, most recently from 8e5de34 to 5e27812 Compare April 19, 2023 07:22
Copy link
Contributor

@smadbe smadbe left a comment

Choose a reason for hiding this comment

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

looks good

@@ -37,11 +37,13 @@ func FeatureContext(s *godog.Suite) {
s.Step(`^there is a group (@\w+)$`, ctx.ThereIsAGroup)
s.Step(`^I am a member of the group (@\w+)$`, ctx.IAmAMemberOfTheGroup)
s.Step(`^I am a member of the group with id "([^"]*)"$`, ctx.IAmAMemberOfTheGroupWithID)
s.Step(`^there are the following group members:$`, ctx.ThereAreTheFollowingGroupMembers)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this rule used anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, indeed I removed it

@GeoffreyHuck GeoffreyHuck merged commit 0068b22 into master Apr 24, 2023
@GeoffreyHuck GeoffreyHuck deleted the getThreadsIsMine branch April 24, 2023 05:38
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.

None yet

2 participants