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

Feature/add actions to detail view #508

Merged
merged 16 commits into from
Jul 11, 2023

Conversation

EdoStorm96
Copy link
Contributor

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.

…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.
@EdoStorm96 EdoStorm96 linked an issue Jul 3, 2023 that may be closed by this pull request
Copy link
Member

@tymees tymees left a 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?

Comment on lines 223 to 224
'''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.'''
Copy link
Member

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.

Comment on lines 247 to 250
'''
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.
'''
Copy link
Member

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

Copy link
Member

@tymees tymees Jul 3, 2023

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.
@EdoStorm96
Copy link
Contributor Author

@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:

proposal.in_archive = False

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:

        committee = proposal.reviewing_committee.name
        return reverse('proposals:archive', args=[committee])

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.

@tymees
Copy link
Member

tymees commented Jul 3, 2023

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

@EdoStorm96
Copy link
Contributor Author

thank you, Ty ;) I figured there was something strange going on ... Take it easy!

Copy link
Contributor

@miggol miggol left a 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!

reviews/utils/review_actions.py Outdated Show resolved Hide resolved
reviews/utils/review_actions.py Outdated Show resolved Hide resolved
@miggol
Copy link
Contributor

miggol commented Jul 6, 2023

I will write up a quick how-to on on merging translations in the meta repo.

@EdoStorm96
Copy link
Contributor Author

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?

@EdoStorm96 EdoStorm96 requested review from miggol and tymees July 10, 2023 08:00
@miggol
Copy link
Contributor

miggol commented Jul 11, 2023

The django.mo file is the compiled version of django.po. It gets generated by python manage.py compilemessages, which is automatically run on the server after every deployment. So as long as django.po is in order, you can ignore django.mo safely. You could put it in your local .gitignore so changes to it aren't recorded or staged.

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.

Copy link
Contributor

@miggol miggol left a 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!

@tymees tymees mentioned this pull request Jul 11, 2023
@EdoStorm96 EdoStorm96 merged commit 8840e23 into develop Jul 11, 2023
1 check passed
@miggol miggol deleted the feature/add_actions_to_detail_view branch November 21, 2023 14:52
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.

Show more actions on detail page
3 participants