Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

Test for view /shift/view_volunteer_shifts/ failing #35

Closed
jayesh92 opened this issue Jun 6, 2015 · 16 comments
Closed

Test for view /shift/view_volunteer_shifts/ failing #35

jayesh92 opened this issue Jun 6, 2015 · 16 comments
Labels
Type: Bug Bug or Bug fixes. Type: Question Further information is requested.

Comments

@jayesh92
Copy link
Contributor

jayesh92 commented Jun 6, 2015

I'm writing test to check if admin can access a volunteer view or not, but the build fails. The test can be viewed at jayesh92/vms@45a3993#diff-0df96ededa86aa47a9dbdb5655a6908aR177 . I've created an admin account and tried to browse url /shift/view_volunteer_shifts/1 which is meant to be a volunteer view. I get a 500 error instead of 403. The code for view being tested is https://github.com/systers/vms/blob/develop/vms/shift/views.py#L285 . The problem here is at line 290, since it's an admin account it's volunteer field will be None and we can't access its 'id'. It would be better if we first check if its not a volunteer and then raise a 403 or better we create a decorator to check admin and use this for all volunteer views.

@Nerdylicious @willingc @vubo

@willingc
Copy link
Contributor

willingc commented Jun 6, 2015

@jayesh92 Thanks for opening an issue. Nice sharing of knowledge and teamwork.

Some quick tips for all on issue communications:

  • Think about the reader's perspective. Can I, as the reader, quickly and easily understand the issue? Where to look for details? Remember many developers are actively working on multiple projects. This requires a developer to do a mental context switch each time they switch projects. Remember that you know the details of the issue but the project maintainer may need a bit of higher level background.
  • Try to use the GitHub markdown to make the issue more user friendly (good job to @jayesh92 and @vubo on this)
  • Break up into different paragraphs:
    • high level issue,
    • steps (if known to replicate issue),
    • possible cause (if known),
    • possible next actions for collaboration on issue

I'm going to reformat the original issue to see if I have understanding of the issue.

@willingc
Copy link
Contributor

willingc commented Jun 6, 2015

The functional test "check if admin can access a volunteer view" is failing. The test can be viewed at jayesh92@45a3993#diff-0df96ededa86aa47a9dbdb5655a6908aR177 .

I've created an admin account and tried to browse url /shift/view_volunteer_shifts/1 which is meant to be a volunteer view. I get a 500 error instead of 403.

The code for view being tested is https://github.com/systers/vms/blob/develop/vms/shift/views.py#L285 .

Suggestions: The problem here is at line 290, since it's an admin account it's volunteer field will be None and we can't access its 'id'. It would be better if we first check if its not a volunteer and then raise a 403 or better we create a decorator to check admin and use this for all volunteer views.

[My reformatting of @jayesh92's original issue with two minor edits]

@willingc
Copy link
Contributor

willingc commented Jun 6, 2015

Questions on the vms project functionality:

  • What is the purpose of view_volunteer_shifts?
  • How does an admin view volunteer shifts of another volunteer?
  • What happens if an admin is also a volunteer?

My recommendation, before addressing the testing issue, would be to address these questions and create a docstring for the function (view_volunteer_shifts) to clarify expected behavior of the function.

After that, I recommend @vubo and @jayesh92 work together to determine if the desired behaviors are being covered properly by functional tests and create a plan to do so.

Thanks!

@willingc willingc added the Type: Question Further information is requested. label Jun 6, 2015
@jayesh92
Copy link
Contributor Author

jayesh92 commented Jun 6, 2015

  • view_volunteer_shifts shows the shifts which the volunteer has either registered for or have been assigned by an admin. He can cancel or log hours from this view.
  • Admin can generate report to see logged in hours of a shift.
  • Not sure on this

@Nerdylicious can tell more about the desired functionality about this issue.

@Nerdylicious
Copy link
Contributor

Hi everyone,

  • Jayesh is on the right track with how to fix this issue (as he described above). I didn't have time to fix this part before GSOC. There may be other views with a similar issue as this that must be fixed.
  • The admin can view the shifts of a volunteer when they manage the shifts of the volunteer (this functionality is in manage_volunteer_shifts in /shift/views.py
  • For now, only administrators are only allowed to manage (create/update/delete) events, jobs, shifts and pull reports. You might want to ask Rose if she wants admins to also be volunteers.

@vubo
Copy link
Contributor

vubo commented Jun 7, 2015

Thank you @jayesh92 for opening an issue. I will get around to it. This part is also in my timeline (22 June-19 July).

@rosariorobinson could you tell please, can admins also be volunteers? And if yes, what should happen if an admin is also a volunteer?

@willingc
Copy link
Contributor

@jayesh92 I'm reviewing open issues. I noticed that your travis.yml file is using Python 2.7. We are currently using Python 3.4 for development. You may wish to add that to your travis file.

@jayesh92
Copy link
Contributor Author

@willingc I'll add that

@willingc
Copy link
Contributor

Thanks @jayesh92. It may help catch some issues sooner versus later. Also Travis can test both Python versions if desired.

@jayesh92 jayesh92 added the Type: Bug Bug or Bug fixes. label Jun 15, 2015
@jayesh92
Copy link
Contributor Author

@willingc We can configure travis to run for PY2 and PY3. I guess it would be good to do so once we have a good compatibility file to take care of differences in coding practices of PY2 and PY3

@rosariorobinson
Copy link
Contributor

@vubo ... Yes, admins can be volunteers. Because they are admins, they should still be able to visit a volunteer job/position and sign up as they were just a volunteer. I think I see admins as just a different view than others with additional functionality. They should be able to view VMS as a volunteer as well.

Let me know if you need more clarity.

@willingc
Copy link
Contributor

@jayesh92 @vubo Can this issue be closed out or are the tests still failing?

@vubo
Copy link
Contributor

vubo commented Jul 27, 2015

@willingc yes, I fixed it: #99

@willingc
Copy link
Contributor

Thanks @vubo. I am closing this issue based on latest merged PR #99.

@jayesh92 Feel free to reopen if needed.

@jayesh92
Copy link
Contributor Author

fine by my side

@willingc
Copy link
Contributor

Thanks @jayesh92 :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Bug Bug or Bug fixes. Type: Question Further information is requested.
Projects
None yet
Development

No branches or pull requests

5 participants