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

Assessment Detail/Value adjustments #1028

Merged
merged 13 commits into from
Jun 5, 2024
Merged

Conversation

munnsmunns
Copy link
Collaborator

@munnsmunns munnsmunns commented May 14, 2024

2 small changes to Assessment details and values:

  • change max length for assessment_details.qa_id from 16 to 32
  • remove validation for assessment_value.value. Now it will display a message on the detail page for the value if the validation doesn't pass

image

Copy link
Owner

@shapiromatron shapiromatron left a comment

Choose a reason for hiding this comment

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

Looking good; a few minor edits for cleanup

Copy link
Owner

@shapiromatron shapiromatron left a comment

Choose a reason for hiding this comment

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

LGTM - I made some edits; please see my comments, as I think you could have caught a few of these w/o me (eg., the migration)

@@ -533,7 +533,7 @@ class AssessmentDetail(models.Model):
default=constants.PeerReviewType.NONE,
)
qa_id = models.CharField(
max_length=16,
max_length=32,
Copy link
Owner

Choose a reason for hiding this comment

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

if you change the model, you have to write a django migration, specifically for changes which alter the SQL schema, as this one would; I went ahead and created one.

@@ -419,6 +420,13 @@ class AssessmentValueDetail(BaseDetail):
def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
context["adaf_footnote"] = constants.ADAF_FOOTNOTE
# add a message if POD/UF is not equal to the value
obj = self.object
Copy link
Owner

Choose a reason for hiding this comment

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

I moved this code to the model itself and not the view, since it's more a property of the object itself rather than the view

{% if obj_perms.edit and value_warning %}
{% alert warning %}
The POD/UF = Value is different than what is reported.
({{object.pod_value|floatformat:'-3'}}/{{object.uncertainty|floatformat:'-3'}} = {{value_warning|floatformat:'-3'}}, which is different than
Copy link
Owner

Choose a reason for hiding this comment

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

I didn't know about this one until now, but if you use stringformat instead of floatformat, you can use a lot more options; honestly the floatformat seems pretty limited for our scientific use case https://docs.python.org/3/library/string.html#format-string-syntax

@shapiromatron shapiromatron merged commit 7cad399 into main Jun 5, 2024
6 checks passed
@shapiromatron shapiromatron deleted the assessment-detail-edits branch June 5, 2024 20:39
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.

2 participants