GO-2969 reorder relation options #1038
Conversation
added RPC and relations: relationOptionOrder/relationOptionInternalOrder
…t into go-2969-reorder-relation-options # Conflicts: # clientlibrary/service/service.pb.go # go.mod # pb/commands.pb.go # pb/service/service.pb.go
…t into go-2969-reorder-relation-options # Conflicts: # clientlibrary/service/service.pb.go # go.mod # go.sum # pb/commands.pb.go # pb/service/service.pb.go # pkg/lib/bundle/relation.gen.go # pkg/lib/bundle/relations.json
Coverage provided by https://github.com/seriousben/go-patch-cover-action |
…t into go-2969-reorder-relation-options # Conflicts: # clientlibrary/service/service.pb.go # pb/commands.pb.go # pb/service/service.pb.go # pkg/lib/bundle/relation.gen.go
| @@ -685,6 +686,51 @@ func (s *Service) RemoveListOption(optionIds []string, checkInObjects bool) erro | |||
| return nil | |||
| } | |||
There was a problem hiding this comment.
Need to extract this code from the block service
…t into go-2969-reorder-relation-options # Conflicts: # clientlibrary/service/service.pb.go # go.mod # go.sum # pb/commands.pb.go # pb/service/service.pb.go # pkg/lib/bundle/relation.gen.go # pkg/lib/bundle/systemRelations.gen.go
WalkthroughThe updates introduced a mechanism for handling and reordering relation options within smart blocks. This includes adding new methods for moving options, retrieving sort keys, and modifying details related to relation options. The changes span various components, including core logic, protobuf files, and new dependencies for lexicographic sorting. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Server
participant Service
client->>Server: Send MoveOption Request
Server->>Service: Call MoveOption
Service->>Service: reorderRelationOption
activate Service
Service-->>Server: Return new order
deactivate Service
Server-->>Client: Send MoveOption Response
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (7)
clientlibrary/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.godocs/proto.mdis excluded by!**/proto.mdgo.sumis excluded by!**/*.sumpb/commands.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopb/service/service.pb.gois excluded by!**/*.pb.go,!**/*.pb.gopkg/lib/bundle/relation.gen.gois excluded by!**/*.gen.gopkg/lib/bundle/systemRelations.gen.gois excluded by!**/*.gen.go
Files selected for processing (8)
- core/block/editor/smartblock/smartblock.go (1 hunks)
- core/block/service.go (3 hunks)
- core/relations.go (1 hunks)
- go.mod (1 hunks)
- pb/protos/commands.proto (1 hunks)
- pb/protos/service/service.proto (1 hunks)
- pkg/lib/bundle/relations.json (1 hunks)
- pkg/lib/bundle/systemRelations.json (1 hunks)
Files skipped from review due to trivial changes (1)
- go.mod
Additional context used
GitHub Check: lint
core/block/service.go
[failure] 674-674:
undefined: Do
[failure] 678-678:
assignment mismatch: 3 variables but s.objectStore.Query returns 2 values (typecheck)
[failure] 674-674:
undefined: Do
[failure] 678-678:
assignment mismatch: 3 variables but s.objectStore.Query returns 2 values) (typecheck)
[failure] 674-674:
undefined: Do
[failure] 678-678:
assignment mismatch: 3 variables but s.objectStore.Query returns 2 values) (typecheck)
Additional comments not posted (5)
pkg/lib/bundle/systemRelations.json (1)
38-39: The addition of "relationOptionOrder" and "relationOptionInternalOrder" fields aligns with the PR's objectives of supporting relation option reordering. The fields are correctly placed and maintain the JSON structure integrity.pb/protos/service/service.proto (1)
129-129: The addition ofRelationMoveOptionRPC method is well-placed and aligns with the PR objectives.Verification successful
The addition of the
RelationMoveOptionRPC method is well-placed and aligns with the PR objectives. The integration and handling of this RPC method are evident in various parts of the codebase, confirming its proper implementation.
pb/service/service.pb.go: Definition and usage ofRelationMoveOption.pb/commands.pb.go: Related request and response types.core/relations.go: Handling of theRelationMoveOptionmethod.clientlibrary/service/service.pb.go: Client-side handling of theRelationMoveOptionmethod.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of the new RPC method `RelationMoveOption` with the rest of the system. # Test: Search for usages of `RelationMoveOption` in the codebase. Expect: Proper implementation and handling in related services. rg --type go 'RelationMoveOption'Length of output: 14749
pkg/lib/bundle/relations.json (1)
91-100: The new field"relationOptionOrder"has been added correctly with appropriate attributes. It is set to be derived, which aligns with the PR description stating that the order includes a fallback to the creation date.core/block/editor/smartblock/smartblock.go (1)
1430-1437: Ensure the new relation option ordering logic matches the expected behavior.pb/protos/commands.proto (1)
2942-2968: TheMoveOptionmessage and its nested structures have been added to handle the reordering of options within a relation. Ensure that theOptionsSortenum correctly reflects the intended sorting behavior as described. The error handling within theResponsemessage is standard and aligns with existing patterns in the codebase. However, consider adding more specific error codes related to the move operation to enhance troubleshooting and user feedback.
| { | ||
| "description": "Relation option internal order", | ||
| "format": "longtext", | ||
| "hidden": true, | ||
| "key": "relationOptionInternalOrder", | ||
| "maxCount": 1, | ||
| "name": "Relation option internal order", | ||
| "readonly": false, | ||
| "source": "details" | ||
| }, |
There was a problem hiding this comment.
The addition of "relationOptionInternalOrder" seems to provide internal sorting capabilities. However, it's marked as non-readonly which could imply that clients might modify it directly. This could lead to inconsistencies if not managed properly.
Consider setting "relationOptionInternalOrder" as readonly to prevent direct modifications, ensuring consistency in the sorting mechanism.
| func (mw *Middleware) RelationMoveOption(cctx context.Context, request *pb.RpcRelationMoveOptionRequest) *pb.RpcRelationMoveOptionResponse { | ||
| response := func(code pb.RpcRelationMoveOptionResponseErrorCode, err error) *pb.RpcRelationMoveOptionResponse { | ||
| if err != nil { | ||
| return &pb.RpcRelationMoveOptionResponse{ | ||
| Error: &pb.RpcRelationMoveOptionResponseError{ | ||
| Code: code, | ||
| Description: err.Error(), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| return &pb.RpcRelationMoveOptionResponse{ | ||
| Error: &pb.RpcRelationMoveOptionResponseError{ | ||
| Code: code, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| err := mw.doBlockService(func(bs *block.Service) error { | ||
| var err error | ||
| if request.Sort == pb.RpcRelationMoveOptionRequest_NewerFirst { | ||
| request.BeforeId, request.AfterId = request.AfterId, request.BeforeId | ||
| } | ||
| err = bs.MoveOption(request.OptionId, request.AfterId, request.BeforeId) | ||
| return err | ||
| }) | ||
| if err != nil { | ||
| return response(pb.RpcRelationMoveOptionResponseError_UNKNOWN_ERROR, err) | ||
| } | ||
|
|
||
| return response(pb.RpcRelationMoveOptionResponseError_NULL, nil) | ||
| } |
There was a problem hiding this comment.
The RelationMoveOption function is implemented to handle the logic for moving relation options. It uses a helper function to swap the IDs based on the sort condition, which aligns with the PR objectives. Consider adding more detailed error descriptions in the response for better clarity and debugging.
- Description: err.Error(),
+ Description: "Error moving relation option: " + err.Error(),Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (mw *Middleware) RelationMoveOption(cctx context.Context, request *pb.RpcRelationMoveOptionRequest) *pb.RpcRelationMoveOptionResponse { | |
| response := func(code pb.RpcRelationMoveOptionResponseErrorCode, err error) *pb.RpcRelationMoveOptionResponse { | |
| if err != nil { | |
| return &pb.RpcRelationMoveOptionResponse{ | |
| Error: &pb.RpcRelationMoveOptionResponseError{ | |
| Code: code, | |
| Description: err.Error(), | |
| }, | |
| } | |
| } | |
| return &pb.RpcRelationMoveOptionResponse{ | |
| Error: &pb.RpcRelationMoveOptionResponseError{ | |
| Code: code, | |
| }, | |
| } | |
| } | |
| err := mw.doBlockService(func(bs *block.Service) error { | |
| var err error | |
| if request.Sort == pb.RpcRelationMoveOptionRequest_NewerFirst { | |
| request.BeforeId, request.AfterId = request.AfterId, request.BeforeId | |
| } | |
| err = bs.MoveOption(request.OptionId, request.AfterId, request.BeforeId) | |
| return err | |
| }) | |
| if err != nil { | |
| return response(pb.RpcRelationMoveOptionResponseError_UNKNOWN_ERROR, err) | |
| } | |
| return response(pb.RpcRelationMoveOptionResponseError_NULL, nil) | |
| } | |
| func (mw *Middleware) RelationMoveOption(cctx context.Context, request *pb.RpcRelationMoveOptionRequest) *pb.RpcRelationMoveOptionResponse { | |
| response := func(code pb.RpcRelationMoveOptionResponseErrorCode, err error) *pb.RpcRelationMoveOptionResponse { | |
| if err != nil { | |
| return &pb.RpcRelationMoveOptionResponse{ | |
| Error: &pb.RpcRelationMoveOptionResponseError{ | |
| Code: code, | |
| Description: "Error moving relation option: " + err.Error(), | |
| }, | |
| } | |
| } | |
| return &pb.RpcRelationMoveOptionResponse{ | |
| Error: &pb.RpcRelationMoveOptionResponseError{ | |
| Code: code, | |
| }, | |
| } | |
| } | |
| err := mw.doBlockService(func(bs *block.Service) error { | |
| var err error | |
| if request.Sort == pb.RpcRelationMoveOptionRequest_NewerFirst { | |
| request.BeforeId, request.AfterId = request.AfterId, request.BeforeId | |
| } | |
| err = bs.MoveOption(request.OptionId, request.AfterId, request.BeforeId) | |
| return err | |
| }) | |
| if err != nil { | |
| return response(pb.RpcRelationMoveOptionResponseError_UNKNOWN_ERROR, err) | |
| } | |
| return response(pb.RpcRelationMoveOptionResponseError_NULL, nil) | |
| } |
| func (s *Service) MoveOption(optionId string, afterId, beforeId string) error { | ||
| return Do(s, optionId, func(b smartblock.SmartBlock) error { | ||
| st := b.NewState() | ||
| relKey := pbtypes.GetString(st.Details(), bundle.RelationKeyRelationKey.String()) | ||
|
|
||
| records, _, err := s.objectStore.Query(database.Query{ | ||
| Filters: []*model.BlockContentDataviewFilter{ | ||
| { | ||
| Condition: model.BlockContentDataviewFilter_Equal, | ||
| RelationKey: bundle.RelationKeySpaceId.String(), | ||
| Value: pbtypes.String(b.SpaceID()), | ||
| }, | ||
| { | ||
| Condition: model.BlockContentDataviewFilter_Equal, | ||
| RelationKey: bundle.RelationKeyRelationKey.String(), | ||
| Value: pbtypes.String(relKey), | ||
| }, | ||
| { | ||
| Condition: model.BlockContentDataviewFilter_Equal, | ||
| RelationKey: bundle.RelationKeyLayout.String(), | ||
| Value: pbtypes.Int64(int64(model.ObjectType_relationOption)), | ||
| }, | ||
| }, | ||
| Sorts: []*model.BlockContentDataviewSort{ | ||
| { | ||
| RelationKey: bundle.RelationKeyRelationOptionOrder.String(), | ||
| }, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("query options: %w", err) | ||
| } | ||
|
|
||
| if len(records) == 0 { | ||
| return fmt.Errorf("target option not found") | ||
| } | ||
|
|
||
| newOrder, err := reorderRelationOption(records, optionId, afterId, beforeId) | ||
| st.SetDetail(bundle.RelationKeyRelationOptionInternalOrder.String(), pbtypes.String(newOrder)) | ||
|
|
||
| return b.Apply(st) | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
The MoveOption function contains several issues:
- The
Dofunction is undefined. You may need to replace it with the correct function name or define it if missing. - There's an assignment mismatch in the
Querycall on line 678.
Consider the following fixes:
- return Do(s, optionId, func(b smartblock.SmartBlock) error {
+ return s.doBlockService(func(b smartblock.SmartBlock) error {
- records, _, err := s.objectStore.Query(database.Query{
+ records, err := s.objectStore.Query(database.Query{Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *Service) MoveOption(optionId string, afterId, beforeId string) error { | |
| return Do(s, optionId, func(b smartblock.SmartBlock) error { | |
| st := b.NewState() | |
| relKey := pbtypes.GetString(st.Details(), bundle.RelationKeyRelationKey.String()) | |
| records, _, err := s.objectStore.Query(database.Query{ | |
| Filters: []*model.BlockContentDataviewFilter{ | |
| { | |
| Condition: model.BlockContentDataviewFilter_Equal, | |
| RelationKey: bundle.RelationKeySpaceId.String(), | |
| Value: pbtypes.String(b.SpaceID()), | |
| }, | |
| { | |
| Condition: model.BlockContentDataviewFilter_Equal, | |
| RelationKey: bundle.RelationKeyRelationKey.String(), | |
| Value: pbtypes.String(relKey), | |
| }, | |
| { | |
| Condition: model.BlockContentDataviewFilter_Equal, | |
| RelationKey: bundle.RelationKeyLayout.String(), | |
| Value: pbtypes.Int64(int64(model.ObjectType_relationOption)), | |
| }, | |
| }, | |
| Sorts: []*model.BlockContentDataviewSort{ | |
| { | |
| RelationKey: bundle.RelationKeyRelationOptionOrder.String(), | |
| }, | |
| }, | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("query options: %w", err) | |
| } | |
| if len(records) == 0 { | |
| return fmt.Errorf("target option not found") | |
| } | |
| newOrder, err := reorderRelationOption(records, optionId, afterId, beforeId) | |
| st.SetDetail(bundle.RelationKeyRelationOptionInternalOrder.String(), pbtypes.String(newOrder)) | |
| return b.Apply(st) | |
| }, | |
| ) | |
| } | |
| func (s *Service) MoveOption(optionId string, afterId, beforeId string) error { | |
| return s.doBlockService(func(b smartblock.SmartBlock) error { | |
| st := b.NewState() | |
| relKey := pbtypes.GetString(st.Details(), bundle.RelationKeyRelationKey.String()) | |
| records, err := s.objectStore.Query(database.Query{ | |
| Filters: []*model.BlockContentDataviewFilter{ | |
| { | |
| Condition: model.BlockContentDataviewFilter_Equal, | |
| RelationKey: bundle.RelationKeySpaceId.String(), | |
| Value: pbtypes.String(b.SpaceID()), | |
| }, | |
| { | |
| Condition: model.BlockContentDataviewFilter_Equal, | |
| RelationKey: bundle.RelationKeyRelationKey.String(), | |
| Value: pbtypes.String(relKey), | |
| }, | |
| { | |
| Condition: model.BlockContentDataviewFilter_Equal, | |
| RelationKey: bundle.RelationKeyLayout.String(), | |
| Value: pbtypes.Int64(int64(model.ObjectType_relationOption)), | |
| }, | |
| }, | |
| Sorts: []*model.BlockContentDataviewSort{ | |
| { | |
| RelationKey: bundle.RelationKeyRelationOptionOrder.String(), | |
| }, | |
| }, | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("query options: %w", err) | |
| } | |
| if len(records) == 0 { | |
| return fmt.Errorf("target option not found") | |
| } | |
| newOrder, err := reorderRelationOption(records, optionId, afterId, beforeId) | |
| st.SetDetail(bundle.RelationKeyRelationOptionInternalOrder.String(), pbtypes.String(newOrder)) | |
| return b.Apply(st) | |
| }, | |
| ) | |
| } |
Tools
GitHub Check: lint
[failure] 674-674:
undefined: Do
[failure] 678-678:
assignment mismatch: 3 variables but s.objectStore.Query returns 2 values (typecheck)
[failure] 674-674:
undefined: Do
[failure] 678-678:
assignment mismatch: 3 variables but s.objectStore.Query returns 2 values) (typecheck)
[failure] 674-674:
undefined: Do
[failure] 678-678:
assignment mismatch: 3 variables but s.objectStore.Query returns 2 values) (typecheck)
This PR adds an ability for clients to reorder the relation's options.
It's done in a way that default order is the date of creation (based on the BSON ID sorting)
RPC added:
RelationMoveOption.relationOptionOrder ASC(for select), set the Sort to 0(OlderFirst).relationOptionOrder DESC(for multi-select tags), set the Sort to 1(NewerFirst).Relations added:
relationOptionOrder,relationOptionInternalOrderClient should use
relationOptionOrderfor sorting because it contains the fallback to the creation dateUnder the hood it is using the lexicographical sorting. It allows to change the position of a single element without having a centralised list of options. There is no need for any migration, because we already use BSON ID for option ids, and bson ids are sorted in the chronological way.
When you add the new option it's automatically will be added to the begin or the end (depends on the sort) because of bsons