4174 Add Last Distribution Date column on the Organization view for Super Admin user. #4207
Conversation
… model to display in the _list.html.erb view for the Superadmin role.
|
Removing |
pshong79
changed the title
| @@ -264,6 +264,11 @@ def earliest_reporting_year | |||
| year | |||
| end | |||
|
|
|||
| def last_distribution_date | |||
| distribution = distributions.order(issued_at: :desc).first | |||
There was a problem hiding this comment.
I would call this display_last_distribution_date rather than last_distribution_date. Naming it last_distribution_date implies that it is a date, to me, anyway.
There was a problem hiding this comment.
Sounds good to me. I've made this change and have pushed up the changes.
| it { expect(response).to be_successful } | ||
|
|
||
| it 'organization details' do | ||
| expect(assigns(:organization)).to have_attributes( |
There was a problem hiding this comment.
I don't see this spec as super helpful - you're diving way deep into the weeds of implementation. I'd rather look at the rendered text and verify that the value you expect to see is there. In this case you should use travel_to to lock in the date and time, update the organization to have specified values, and then expect those values to be in the output.
There was a problem hiding this comment.
Ok I will take a look at this again. Sorry, I have not forgotten about this. I just got a new Macbook and am still in the process of getting things up and running!
There was a problem hiding this comment.
@dorner Starting to take a look at this now and trying to wrap my head around your suggestion. Sorry, I'm still relatively newer and still learning a lot. Do you know if there is something similar or an example I can look at?
There was a problem hiding this comment.
Here's an example of the traveling:
and here's how you can check the rendered text:
You can get more nitty-gritty by actually turning the text into DOM and checking it specifically:
There was a problem hiding this comment.
I'm not sure we actually need to travel-to to test this out -- we're looking at the issued_at, rather than the created_at for the last distribution date, and that is "manually" set. Or am I missing something?
There was a problem hiding this comment.
I've kind of been struggling with how to use travel_to with the issued_at myself...
There was a problem hiding this comment.
My bad - as usual @cielf is right 😃
There was a problem hiding this comment.
So can I resolve this conversation and we're good to go with this change?
There was a problem hiding this comment.
No - it's still an issue IMO. You shouldn't be looking at assigns - it's better to look at the rendered text.
There was a problem hiding this comment.
This took waaaay longer than I had hoped and expected. I think I figured it out though and, of course, it seems to be way simpler than I had made it out to be.
Sorry, that it took so long.... still learning over here 🙃
|
Nice, thanks! |
|
@pshong79: Your PR |
Resolves #4174
Description
This change adds the
Last Distribution Dateto the Organization view for the Super Admin user.Type of change
How Has This Been Tested?
organization_requests_spec.rbfile to verify all the data returned for an organization, including thelast_distribution_dateScreenshots
Before:
After:
