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

Fix/post testing requests #818

Merged
merged 18 commits into from
Dec 18, 2024
Merged

Fix/post testing requests #818

merged 18 commits into from
Dec 18, 2024

Conversation

EdoStorm96
Copy link
Contributor

@EdoStorm96 EdoStorm96 commented Dec 10, 2024

This branch features a lot of tiny issues, that have been uncovered following the testing of 4.0.

In rough order of commits:

#789: Apparently the observation table still included fields from v1 observations and we never noticed. It now includes the new fields.

#809: The Stepper bubbles were untranslated ... My first hunch was to replace gettext with gettext_lazy and this works. Don't ask me why ...

#811: Increase the max length of trajectory names from 15 to 30.

#814: Include linebreaks in review comments for the Secretary to read comfortably.

No issue: Remove more remnant of the old AvailableURL system ...

#813: Untranslated text.

#817: A server errors caused by some shoddy validation and logic regarding embargo and embargo_end_date.

#777: Remove the embargo question from the submit form for PreAss proposals, ensure that PreAss proposals are never placed in the archive and remove the ChangeArchiveStatus review action if a proposal is PreAss.

Its a lot of issues ... But the fixes are mostly one or two lines, so it should be manageable I hope :)

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.

Awesome you got to all of this. Changes requested are minor

Copy link
Contributor

Choose a reason for hiding this comment

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

Double check migrations before merging

@@ -52,7 +52,7 @@ class Observation(SettingModel):
)

details_frequency = models.TextField(
_("Beschrijf <b>hoe vaak en hoe lang</b> de observant wordt geobserveerd."),
_("Beschrijf <b>hoe vaak en hoelang</b> de observant wordt geobserveerd."),
Copy link
Contributor

Choose a reason for hiding this comment

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

Still looks weird to me. But it is correct apparently

@@ -245,14 +245,17 @@ def is_available(self):
return False

if (
review.proposal.embargo == True
review.proposal.embargo_end_date is not None
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know we don't reset embargo_end_date when an applicant sets embargo back to False. So there are potentially proposals with no embargo but yes embargo_end_date.

Therefore you must check both here.

@@ -17,7 +17,7 @@
<td>{{ decision.reviewer.get_full_name }}</td>
<td>{{ decision.get_go_display|default:_("open") }}</td>
<td data-order="{{ decision.date_decision|date:'c' }}">{{ decision.date_decision|date:"j F Y, G:i" }}</td>
<td>{{ decision.comments }}</td>
<td>{{ decision.comments | linebreaks }}</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

I never knew about this filter. Now I feel like an idiot for <br />ifying everything by hand in grant-tool. Good find

@EdoStorm96 EdoStorm96 merged commit 37f2d87 into major/4 Dec 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment