-
-
Notifications
You must be signed in to change notification settings - Fork 6
SF-3486 Hide completed drafts when content is not available #3348
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3348 +/- ##
=======================================
Coverage 82.42% 82.42%
=======================================
Files 607 607
Lines 35571 35579 +8
Branches 5778 5799 +21
=======================================
+ Hits 29319 29327 +8
+ Misses 5389 5373 -16
- Partials 863 879 +16 ☔ View full report in Codecov by Sentry. |
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.
Another requirement on the Jira issue is that the notice would not show for projects older newer than Dec. 3, but I am not sure how to implement that since the project docs do not explicitly have a date created property.
Figuring out how to deal with that would be a good thing to bring up with the team in Slack.
Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @pmachapman)
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.
Another requirement on the Jira issue is that the notice would not show for projects older newer than Dec. 3, but I am not sure how to implement that since the project docs do not explicitly have a date created property.
You could parse the project doc id to get the timestamp the project was created?
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pmachapman)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-list.component.ts
line 5 at r1 (raw file):
import { TranslocoModule, TranslocoService } from '@ngneat/transloco'; import { take } from 'rxjs'; import { NoticeComponent } from 'src/app/shared/notice/notice.component';
NIT: This should be a relative path.
Code quote:
import { NoticeComponent } from 'src/app/shared/notice/notice.component';
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-list.component.ts
line 8 at r1 (raw file):
import { ActivatedProjectService } from 'xforge-common/activated-project.service'; import { filterNullish, quietTakeUntilDestroyed } from 'xforge-common/util/rxjs-util'; import { I18nService } from '../../../../xforge-common/i18n.service';
NIT: Remove the ../../../../
.
Code quote:
import { I18nService } from '../../../../xforge-common/i18n.service';
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-list.component.ts
line 25 at r1 (raw file):
export class DraftHistoryListComponent { history: BuildDto[] = []; readonly draftHistoryCutoffDate: string = this.i18n.formatDate(new Date('2024-12-03T12:00:00.000Z'));
NIT: Are you able to add a comment to the code as to why this date was chosen, so we know in future when we look at the code again?
Code quote:
readonly draftHistoryCutoffDate: string = this.i18n.formatDate(new Date('2024-12-03T12:00:00.000Z'));
dccaea6
to
6df1e0d
Compare
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.
Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-list.component.ts
line 5 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
NIT: This should be a relative path.
Done.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-list.component.ts
line 8 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
NIT: Remove the
../../../../
.
Done.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-list.component.ts
line 25 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
NIT: Are you able to add a comment to the code as to why this date was chosen, so we know in future when we look at the code again?
Done. I have changed the date to 1 June with comment.
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.
@siltomato Can you please review this PR?
Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @RaymondLuong3)
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.
Just one minor concern about getter usage.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-list.component.ts
line 93 at r2 (raw file):
} get savedHistoricalBuilds(): BuildDto[] {
I know getters have become a common pattern for us, but I feel like a function getSavedHistoricalBuilds()
would be better than a getter here, as we would be more likely to appreciate the work being done by the function and cache the result where necessary. What are your thoughts?
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.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @siltomato)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-list.component.ts
line 93 at r2 (raw file):
Previously, siltomato wrote…
I know getters have become a common pattern for us, but I feel like a function
getSavedHistoricalBuilds()
would be better than a getter here, as we would be more likely to appreciate the work being done by the function and cache the result where necessary. What are your thoughts?
I think since savedHistoricalBuilds
doesn't call any API endpoints, but just filters data that is already in memory it is OK to be a property. If it were loading from and API or LocalDB, or included any kind of expensive operation, I'd be much more inclined to make it a function.
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)
Some drafts appear in the history without a way to use them. This is because they were generated before the draft history feature was introduced. This change hides drafts that were generated but cannot be downloaded or applied because they are not stored in our database. As per the balsamiq I added a notice to inform the user that drafts older than Dec 3 are not available in the history.
Another requirement on the Jira issue is that the notice would not show for projects older newer than Dec. 3, but I am not sure how to implement that since the project docs do not explicitly have a date created property.
This change is