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 history displays irrelevant changes #922

Closed
wants to merge 5 commits into from
Closed

Conversation

thachlp
Copy link
Member

@thachlp thachlp commented Feb 24, 2024

Motivate:
The history of a directory includes changes in directories share the name. For instance: the history of /foo displays changes for /foo2, /foo3, /foobar ...

Modifications:
Add a slash (/) before ** in the path (path + "/**"). The pattern foo/** matches /foo/anything, /foo/subdir/file, but it does not match anything that isn't a subpath of /foo.

Result
Fixes #902

@thachlp thachlp marked this pull request as draft February 24, 2024 16:21
Copy link

codecov bot commented Feb 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.52%. Comparing base (ac1e8c1) to head (0eae756).
Report is 4 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #922      +/-   ##
============================================
+ Coverage     66.78%   67.52%   +0.74%     
- Complexity     3513     3567      +54     
============================================
  Files           370      370              
  Lines         14476    14477       +1     
  Branches       1553     1553              
============================================
+ Hits           9668     9776     +108     
+ Misses         3929     3821     -108     
- Partials        879      880       +1     

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

@thachlp thachlp marked this pull request as ready for review February 25, 2024 01:23
@minwoox minwoox added the defect label Feb 27, 2024
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks! Could you add some tests, please? 😉

@minwoox
Copy link
Member

minwoox commented Feb 27, 2024

Could you also check if ContentServiceV1 correctly works, please?

@thachlp
Copy link
Member Author

thachlp commented Feb 27, 2024

Could you also check if ContentServiceV1 correctly works, please?

Ok, let me check

@thachlp
Copy link
Member Author

thachlp commented Feb 27, 2024

Thanks! Could you add some tests, please? 😉

Sure, let me try.
Hi @minwoox I try to create a CentralDogmaExtension like RepositoryServiceV1Test to test /api/v0/, but it always returns 404, seem it doesn't support /api/v0. Could you help me with this?

@thachlp thachlp closed this Feb 27, 2024
@thachlp thachlp deleted the issue_902 branch February 27, 2024 03:33
@thachlp thachlp restored the issue_902 branch February 27, 2024 03:35
@thachlp thachlp reopened this Feb 27, 2024
final AggregatedHttpResponse userInfo = client.get("/api/v0/users/me").aggregate().join();
final JsonNode jsonNode = Jackson.readTree(userInfo.contentUtf8());
assertThat(jsonNode.get("login").asText()).isEqualTo("[email protected]");
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@minwoox here, I try to add tests for the class RepositoryServiceTest, but it always returns 404 for /api/v0 (I try users/me to make sure I am correct ^^)
Do you know how to start the /api/v0 dogma for the test?

Copy link
Member

Choose a reason for hiding this comment

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

It's activated only when the webAppEnabled is true.


By the way, could you tell me why you need to retrieve the user information?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @minwoox, I can call /api/v0 now.
I just do the simplest test with api/v0 to explain to you my problem 🤩

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Thanks for the initiative! Left a high-level review before going into details 🙇

@@ -177,10 +177,11 @@ public CompletionStage<List<CommitDto>> getHistory(@Param String projectName,
@Param String path,
@Param @Default("-1") String from,
@Param @Default("1") String to) {
final String pathPattern = "/".equals(path) ? "/**" : path + "/**";
Copy link
Contributor

Choose a reason for hiding this comment

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

The other APIs we have (e.g. watch*, diff*) don't add a slash at the end.
For consistency, I think it would be better that all APIs that accept a pathPattern have the same behavior.
Also, I think it is perfectly valid to watch /a** depending on the use-case.

Question) What do you think of fixing this issue from the client side instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @jrhee17, about fixing this issue from the client side, do you mean we receive the list result from the client and filter?
From my side, it is not good to do logic on the client side.
@minwoox How do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry about the late reply. Yeah, I also thought we should fix the UI (which is the client) to call the path correctly when I first created the issue.
We should let users retrieve whatever history they want so I think we should touch it on the server-side.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sound good @minwoox, could you provide me the screenshot to explain expected from client? I am happy to finish this 🚀

@thachlp thachlp closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Directory history displays irrelevant changes
3 participants