-
Notifications
You must be signed in to change notification settings - Fork 4
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
2411 metadata parsed datafiles #2706
Conversation
This reverts commit c5796da.
- Added admin filter to show newest or all datafile records - Updated indices to allow easier elastic queries
- Moved create_datafile to util
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.
Working great for me locally! Let's get those e2e pipeline errors fixed and move this forward, thanks!
@andrew-jameson : thanks for your review Andrew, the tests are now passing |
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.
bit of a nitpick on the custom filter classes but everything is working as expected.
"""Return a list of tuples.""" | ||
return [ | ||
('Accepted', 'Accepted'), | ||
('Rejected', 'Rejected'), |
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.
we have a couple more statuses than just Accepted and Rejected, could they be added? also rather than create a custom filter class, you could add 'datafilesummary__status'
to the DataFileAdmin
's list_filter
- i tested that out quickly and it correctly ports all the DataFileSummary.Status
options without needing to hard code anything
@@ -152,6 +152,20 @@ class Meta: | |||
null=True | |||
) | |||
|
|||
@property | |||
def prog_type(self): |
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.
not really specific to this PR, but similar logic is repeated everywhere and i keep thinking we should have a first-class member for the program type. similar to my other comment this would also allow you to use a native django list filter for the program type without creating a custom 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.
Having prog_type as property means calculation is done in backend server vs DB. While normally querying the DB is faster, there are cases where pushing the query into DB is not efficient since during writing to prog_type need a db_lock. An example is calculated values where property can change base on two, three different fields.
Having said that, in this case since prog_type only needs to be updated during the initial write, I agree with your suggestion
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.
looks great @raftmsohani 🚀 especially like how i can obtain the status details of the file from the data_files api endpoint and also how the parsing errors for each file can be easily accessed by clicking the link in DAC. id like to discuss with @ttran-hub if it would be helpful to add more cols to parser errors table. if so, this will be captured in an enhancement ticket.
Summary of Changes
Provide a brief summary of changes
Pull request closes #2411 _
How to Test
To test, use the following commands to start the app:
You will see the loaded datafiles and the added two columns as shown below:
Deliverables
Added new 'program type' filter to datafile list page, as well as two new columns:
Deliverable 1: Accepted Features
Checklist of ACs:
lfrohlich
and/oradpennington
confirmed that ACs are met.Deliverable 2: Tested Code
CodeCov Report
comment in PR)CodeCov Report
comment in PR)Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
iamjolly
andttran-hub
using Accessibility Insights reveal any errors introduced in this PR?Deliverable 5: Deployed
Deliverable 6: Documented
Deliverable 7: Secure
Deliverable 8: User Research
Research product(s) clearly articulate(s):