Move refresh button and show last refresh timestamp#1361
Move refresh button and show last refresh timestamp#1361sadiqkhoja merged 4 commits intogetodk:masterfrom
Conversation
ca35e54 to
bedd4f9
Compare
matthew-white
left a comment
There was a problem hiding this comment.
I think the main question I have is which timestamp to show: the time that the request was sent or the time that the response was received.
Originally, I was thinking that we would show the time that the response was received. I added setAt and patchedAt to requestData in #1341, and I was thinking that we could use something like max(odata.setAt, odata.patchedAt) here. That would work for SubmissionMapView as well, since SubmissionMapView sets odata after the GeoJSON response is received. The main benefit of that approach is that SubmissionList and EntityList can avoid managing an additional piece of state that requires coordination with child components (the now data property).
Now that I think more about it though, maybe there is a way that showing the time that the response was received could be confusing. If the request takes a long time, then new data could theoretically come in between when the request was sent and the response was received. In that case, if we show the time that the response was received, the user might think that they've received all submissions created before that time — when actually that isn't the case. That would kind of be an assumption on the user's part though: I don't think the wording of "Data last refreshed at..." implies that exactly.
This is just my intuition, but I personally feel like it's a little confusing to immediately update the timestamp that's shown (to update it as soon as the Refresh button is clicked). If the request takes a long time, I wouldn't expect the timestamp to change until after the request completes. It's only then that the data has actually been successfully refreshed. Similarly, if the request fails, I wouldn't expect the timestamp to change at all. If we used max(setAt, patchedAt), that wouldn't change if the request fails.
I feel like both approaches have pros and cons; I can see both sides to it. I'm personally more in favor of showing the time that the response was received. That seems reasonably clear to me, and it'd also help simplify SubmissionList and EntityList, since they wouldn't have to manage now.
src/locales/en.json5
Outdated
| "search": "Search", | ||
| // Text shown above the data tables to tell users when data was last fetched / refreshed. | ||
| // {date} is only the date component and {time} is only the time component. | ||
| "dataRefreshTime": "Data last refreshed at {date} on {time}" |
There was a problem hiding this comment.
| "dataRefreshTime": "Data last refreshed at {date} on {time}" | |
| "dataRefreshTime": "Data last refreshed on {date} at {time}" |
this was my worry when I used |
src/components/entity/list.vue
Outdated
| <div class="table-refresh-info"> | ||
| <span v-show="odataEntities.dataExists"> | ||
| {{ $t('common.dataRefreshTime', { date: formatDate(dataRefreshedAt), time: formatTime(dataRefreshedAt) }) }} | ||
| </span> | ||
| <button id="entity-list-refresh-button" type="button" | ||
| class="btn btn-link" :aria-disabled="refreshing || bulkOperationInProgress" | ||
| @click="fetchChunk(false, true)"> | ||
| <span class="icon-refresh"></span>{{ $t('action.refresh') }} | ||
| <spinner :state="refreshing"/> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
What do you think about turning this into a component? It could emit a click event and receive the following props:
- The OData resource — either
odataorodataEntities(needed for the timestamp and also forv-show="odataEntities.dataExists") refreshingdisabled
I like the idea of a component for a few reasons:
- A single file component would centralize the HTML,
.table-refresh-infostyles, andcommon.dataRefreshTimemessage in a single file. SubmissionListandEntityListare already pretty complicated. If they can offload things like date/time formatting to a child component, that sounds nice to me.- Looking ahead to the next release, I'm thinking that this component would be the one responsible for the logic for Inform user of new submissions/entities central#1332
- Fetching the latest count
- Adjusting the appearance of the refresh button when there are new submissions to pull
It would be a pretty tiny component (at least for now), so I could also see the argument that it's not worth adding one more component. I don't think it'd be any smaller than the OdataLoadingMessage component though.
65bb83a to
120eae7
Compare
ee7cfd4 to
4ae8c60
Compare
4ae8c60 to
92a1529
Compare
src/components/table-refresh-bar.vue
Outdated
| refreshing: Boolean, | ||
| disabled: Boolean |
There was a problem hiding this comment.
we can use just one here or maybe in future we might need two?
There was a problem hiding this comment.
I agree, I think it could just be one for now.
src/locales/en.json5
Outdated
| // Text shown above the data tables to tell users when data was last fetched / refreshed. | ||
| // {date} is only the date component and {time} is only the time component. | ||
| "dataRefreshTime": "Data last refreshed on {date} at {time}" |
There was a problem hiding this comment.
How about moving this into the TableRefreshBar component?
There was a problem hiding this comment.
There's some CSS here that could probably go away:
// Further increase the space between the dropdown and the refresh button.
margin-right: 10px;
Closes getodk/central#1106
What has been done to verify that this works as intended?
Added assertion in the existing tests and manual verification
Why is this the best possible solution? Were any other approaches considered?
I considered creating
nowin SubmissionList and pass it to the children view components as prop but view components also update data independently (using watchers on filter change). Emitting timestamp on refresh felt better approach.Using Luxon DateTime so that it is easy to show timestamp in local timezone and send UTC for snapshot filter.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
No
Does this change require updates to user documentation? If so, please file an issue here and include the link below.
No
Before submitting this PR, please make sure you have:
npm run testandnpm run lintand confirmed all checks still pass OR confirm CircleCI build passes