Skip to content

Bugfix sorting exported reports for project and access permissions#266

Merged
kbeker merged 6 commits intomasterfrom
bugfix-sorting-exported-reports-for-project
Jun 4, 2019
Merged

Bugfix sorting exported reports for project and access permissions#266
kbeker merged 6 commits intomasterfrom
bugfix-sorting-exported-reports-for-project

Conversation

@kbeker
Copy link
Contributor

@kbeker kbeker commented Jun 3, 2019

The point is that when manager exported project with all reports they were not sorted at all. This PR fixes it.

@kbeker kbeker added the bug Something isn't working label Jun 3, 2019
@kbeker kbeker added this to the next_release milestone Jun 3, 2019
@kbeker kbeker self-assigned this Jun 3, 2019
@kbeker
Copy link
Contributor Author

kbeker commented Jun 3, 2019

@rwrzesien I added now one more thing to this PR. Please look at it also because it is related with your PR with permissions.

check_permissions(allowed_user_types=[CustomUser.UserType.ADMIN.name, CustomUser.UserType.MANAGER.name]),
name="dispatch",
)
class ExportReportsInProjectView(DetailView, UserIsManagerOfCurrentReportProjectMixin):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just reverse classes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwrzesien Exactly what I wanted you to see! When they are reversed they don't work :D And I am wondering why :P

Copy link
Contributor

@rwrzesien rwrzesien Jun 4, 2019

Choose a reason for hiding this comment

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

The problem was that instead of UserIsManagerOfCurrentReportProjectMixin it should use UserIsManagerOfCurrentProjectMixin, as this view looks up for Project, not Report.

Please remember to update AccessPermissionsTestCase if you modify anything regarding access permission, add new view etc. This is a remark to everyone involved in @Code-Poets/sheetstorm developing.

self.assertEqual(self.report.description, self.workbook_for_user.active.cell(row=2, column=5).value)


class DataSetUpForSortingTests(TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that as long as this is used only in single subclass, there is no need to keep it in separate class.

@rwrzesien
Copy link
Contributor

@kbeker Thanks to this problem which you have found I have discovered some problems in access permission tests, for which fix, as discussed, I will add to this pull request, so it should resolve both problems. I have also resolved some other small problems with access permissions which I have found along the way.

@kbeker kbeker force-pushed the bugfix-sorting-exported-reports-for-project branch 2 times, most recently from ec0c99f to 0a281c1 Compare June 4, 2019 06:52
@kbeker kbeker changed the title Bugfix sorting exported reports for project Bugfix sorting exported reports for project and access permissions Jun 4, 2019
@kbeker kbeker force-pushed the bugfix-sorting-exported-reports-for-project branch from 0a281c1 to 044b806 Compare June 4, 2019 07:02
@kbeker
Copy link
Contributor Author

kbeker commented Jun 4, 2019

@kbeker Thanks to this problem which you have found I have discovered some problems in access permission tests, for which fix, as discussed, I will add to this pull request, so it should resolve both problems. I have also resolved some other small problems with access permissions which I have found along the way.

I tested one more time everything and I think all works :). Merging

@kbeker kbeker merged commit 044b806 into master Jun 4, 2019
@kbeker kbeker deleted the bugfix-sorting-exported-reports-for-project branch June 4, 2019 07:04
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.

2 participants