Skip to content

Conversation

@xkello
Copy link

@xkello xkello commented Jan 26, 2026

@coveralls
Copy link

coveralls commented Jan 26, 2026

Pull Request Test Coverage Report for Build 21628954614

Details

  • 109 of 113 (96.46%) changed or added relevant lines in 5 files are covered.
  • 21 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 94.394%

Changes Missing Coverage Covered Lines Changed/Added Lines %
server/mergin/sync/permissions.py 8 9 88.89%
server/mergin/sync/public_api_v2_controller.py 18 21 85.71%
Files with Coverage Reduction New Missed Lines %
server/mergin/tests/test_utils.py 1 99.01%
server/mergin/utils.py 20 67.5%
Totals Coverage Status
Change from base Build 21353134513: -0.01%
Covered Lines: 8200
Relevant Lines: 8687

💛 - Coveralls

Copy link
Collaborator

@MarcelGeo MarcelGeo left a comment

Choose a reason for hiding this comment

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

Nice 🎉

  • we need to double check if 404 is ok if you have empty projects list or return error: 404

  • check def get_projects_by_uuids(uuids) method in project_api_controller.py if everything with permissions is ok here

Copy link
Contributor

@harminius harminius left a comment

Choose a reason for hiding this comment

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

Very good job!

I think two things need to be discussed (to fulfill clients' requirements):
1, Shall all (valid) uuids coming from the request be part of the response or can we skip them as suggested here?
2, Do we need to differentiate between 404 and 403 here with the expose flag? I tend to make things easier by keeping only 404 error.

@harminius
Copy link
Contributor

@xkello Although your approach returns a correct response, I think we need to slightly change the approach.
You can keep the require_project_by_many_uuids() function, but when making the response, I would compare the incoming uuids with ones we have found projects for. All the rest should get the error code.
That's how I understand this requirement: https://github.com/MerginMaps/server-private/issues/3155#issuecomment-3804048237

Copy link
Contributor

@harminius harminius left a comment

Choose a reason for hiding this comment

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

Great job, we're almost there! 👍

I have only one major comment.
I have doubts about the usefulness of require_project_by_many_uuids(). At least it should return a list of projects, and the rest (by_id dict creation) should be done in the controller. However, since this function needs to list even removed projects, it doesn't seem reusable.
The idea of the help function is that it can be used in a different use case, while this function is designed for this specific usecase.

Copy link
Contributor

@harminius harminius left a comment

Choose a reason for hiding this comment

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

The last concern is about logged out users not able to list their projects
Otherwise 💚

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.

6 participants