-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/add actions to detail view #508
Conversation
…s not work yet when trying to perform the action.
… reverse function in HideFromArchiveView needed a committee args anyway, but I am not sure if the hide action works at all ... Bit confused. Might leave this for now and look into the confirmation action in detail view
… at the original translations for the hide action, I am more confused about the hide action. Does it add to archive or hide from archive? Left these translations for now ...
…ion. This has improved it, but Michael and me did come to the conclusion that there seem to be problems with the hide functionality in general, which I'll look into some more.
…ctionality is not currently in use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor formatting issues, but otherwise looks good!
but the hide from archive view does not seem to be working correctly.
Could you elaborate?
reviews/utils/review_actions.py
Outdated
'''Only the secretary is able to send the confirmation letter and/or change the date. | ||
The review needs to be closed and have an approved status.''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to right after def is_available(self):
, as it's a docstring.
reviews/utils/review_actions.py
Outdated
''' | ||
This class should lead to the archive_hide url, but the hide functionality does currently not work properly/ is not in use. | ||
Therefore it is commented out in the ReviewActions class. | ||
''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to right after class HideReview(ReviewAction):
, as it's a docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, as the contents are not really docstring like, it should be changed to a normal comment (using #)
In general, '''
strings should only be used for docstrings or multiline string values
… changed docstring placement, as per Ty's request.
@tymees I changed the placement of the docstrings. To elaborate on the working of the HideFromArchiveView, it simply does not hide the proposal from the archives consistently. However, I am also confused about which archive we are talking about with this view. From how I understand it there's the private archive, the url name 'archive'. Here I experience inconsistency in whether the proposal shows up at all after conclusion. furthermore, I have not succeeded in hiding a proposal from this page using the HideFromArchiveView. This view redirects to the private archive, when it is called, which I also find confusing. Then there is the public archive, with the url name 'public_archive'. Here, proposals show up more consistently for me. I also have managed to remove proposals here using the HideFromArchiveView, but only after having added a line, namely:
I have also had to reformat the reverse call at the end of the get_redirect_url method of this view, as that was causing a reverse error from me. I changed it to:
So if the point of the HideFromArchiveView is to remove a proposal from the public archive, then the view was not working correctly, but can be with the changes discussed here. I would suggest however, that it would then redirect to the public archive, to avoid confusion for the user. Now with the private archive, I am experiencing inconsistency in general, which I don't yet fully understand. If the point is to hide a proposal from here, I will need some more explanation on the workings and intention of this page. |
Ugh okay I've figured out what the problem is with the archive stuff. It's not that hard to fix, but it's a bit of a head scratcher to describe. The existance of 4(!) different archives is probably the culprit, which causes confusion on what does what. I'm rapidly approaching my daily work-time-allowence, so I'll write a new issue for this later sometime this week. For now, this PR can go ahead to develop I think without that fix. We'll just enable the corresponding action once it's fixed. There is still the issue of the file conflicts; it's the dreaded translation files again. Michael is our resident PO-file conflict expert, so you might want to hit him up when he's back |
thank you, Ty ;) I figured there was something strange going on ... Take it easy! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some style considerations, otherwise I'm happy!
I will write up a quick how-to on on merging translations in the meta repo. |
All right, I implemented the suggested changes, but there is still a merge conflict caused by the .mo file. What do I do about this? |
The In this case, because changes to it have already been committed, you should resolve the conflict by simply choosing your local (most up-to-date) version as the correct one. I can help you with this if you like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good to go!
Adds the confirmation letter/ change confirmation data actions to the detail view for reviewers. Also created a detail view action class for the hide from archive action, but the hide from archive view does not seem to be working correctly.