-
Notifications
You must be signed in to change notification settings - Fork 33
fix: add auto set end date if only begin date is provided for dataset and proposal filters #2063
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
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `cypress/e2e/proposals/proposals-general.cy.js:593` </location>
<code_context>
+
+ cy.visit("/proposals");
+
+ cy.intercept("GET", "/api/v3/proposals/fullquery*").as("fullquery");
+
+ cy.get('[data-cy="creation-time-begin"]').type("2025-10-08");
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the test by extracting repeated intercept and assertion logic into custom Cypress commands for clarity and maintainability.
```suggestion
Consider collapsing the double `cy.wait()` and extracting the common “intercept & date‐assert” pattern into reusable Cypress commands. This will keep your test focused and DRY:
// cypress/support/commands.js
Cypress.Commands.add('filterByStartDate', (selector, date) => {
cy.intercept('GET', '/api/v3/proposals/fullquery*').as('fullquery')
cy.get(selector).type(date)
cy.get('[data-cy="apply-button-filter"]').click()
})
Cypress.Commands.add('assertAutoEndDate', () => {
const today = new Date().toISOString().slice(0, 10)
cy.wait('@fullquery').then(({ request: { url } }) => {
expect(url).to.match(/\/api\/v3\/proposals\/fullquery/)
expect(url).to.include('startTime')
expect(url).to.include(today)
})
})
-- TEST BEFORE --
cy.intercept("GET", "/api/v3/proposals/fullquery*").as("fullquery")
cy.get('[data-cy="creation-time-begin"]').type("2025-10-08")
cy.get('[data-cy="apply-button-filter"]').click()
cy.wait("@fullquery")
cy.wait("@fullquery").then((interception) => { … })
-- TEST AFTER --
cy.createProposal(newProposal)
cy.visit("/proposals")
cy.filterByStartDate('[data-cy="creation-time-begin"]', '2025-10-08')
cy.assertAutoEndDate()
```
This:
- Removes the duplicate `cy.wait`.
- Pulls the intercept+assert logic into `assertAutoEndDate()`.
- Makes the test body a single, clear flow.
</issue_to_address>
### Comment 2
<location> `cypress/e2e/datasets/datasets-general.cy.js:418` </location>
<code_context>
+
+ cy.intercept("GET", "/api/v3/datasets/fullquery*").as("fullquery");
+
+ cy.wait("@fullquery").then((interception) => {
+ const url = interception.request.url;
+ expect(url).to.contain("/api/v3/datasets/fullquery");
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated intercept and date assertion logic into a custom Cypress command for cleaner, more maintainable tests.
You’re repeating the same intercept/wait + date‐assertion logic. Pull it out into a custom Cypress command so each test just calls a one-liner. For example:
```js
// cypress/support/commands.js
import dayjs from 'dayjs';
Cypress.Commands.add(
'waitAndAssertDateFilter',
(alias, { begin, paramName = 'creationTime' }) => {
const today = dayjs().format('YYYY-MM-DD');
cy.wait(alias).its('request.url').then((url) => {
expect(url).to.contain(`/${paramName}Begin=${begin}`);
expect(url).to.contain(`/${paramName}End=${today}`);
});
}
);
```
Then in your test:
```js
describe('Dataset filter end date auto-set', () => {
it('should set end date to today if only start date is provided', () => {
cy.createDataset({ type: 'raw', creationTime: '2025-10-08T15:00:00.000Z' });
cy.visit('/datasets');
cy.get('[data-cy="creation-time-begin"]').type('2025-10-07');
cy.get('[data-cy="search-button"]').click();
cy.intercept('GET', '/api/v3/datasets/fullquery*').as('fullquery');
// replaces the inlined wait/expect block:
cy.waitAndAssertDateFilter('@fullquery', { begin: '2025-10-07' });
});
});
```
This keeps your tests concise, DRY and avoids nesting/duplication, while preserving all existing assertions.
</issue_to_address>
### Comment 3
<location> `cypress/e2e/datasets/datasets-general.cy.js:419` </location>
<code_context>
const url = interception.request.url;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {url} = interception.request;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 4
<location> `cypress/e2e/proposals/proposals-general.cy.js:601` </location>
<code_context>
const url = interception.request.url;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
const {url} = interception.request;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 5
<location> `src/app/state-management/effects/proposals.effects.ts:46` </location>
<code_context>
const startTime = queryParam.startTime;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {startTime} = queryParam;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
const queryParam = search ? { ...search } : {}; | ||
const { startTime } = queryParam; | ||
if ( | ||
startTime && | ||
typeof startTime === "object" && | ||
startTime !== null && | ||
"begin" in startTime && | ||
"end" in startTime && | ||
startTime.begin && | ||
startTime.end == null | ||
) { | ||
queryParam.startTime = { | ||
...startTime, | ||
end: new Date().toISOString(), | ||
}; | ||
} |
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.
Why is this filter so much more complicated than the one for creationTime in dataset? Which pattern should I follow if I need this check for other components?
const { skip, limit, sortField, modeToggle, ...theRest } = filter; | ||
const modifiedFilters = { ...theRest }; | ||
if ( | ||
modifiedFilters.creationTime?.begin && | ||
modifiedFilters.creationTime.end === null | ||
) { | ||
modifiedFilters.creationTime.end = new Date().toISOString(); | ||
} |
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.
Is it possible to prevent time.begin or time.end to be appended to the query if value is not given in the first place? instead of modifying them on the fly?
Description
This PR ensures that when a user provides only a begin date for dataset or proposal filters, the end date is automatically set to today.
Motivation
If users did not provide an end date and MongoDB was enabled, the end date would be "
1970-01-01T00:00:00.001Z
" instead ofnull.
This resulted in queries that would not return any resultsFixes:
Please provide a list of the fixes implemented in this PR
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Automatically set the end date to today when only a start/begin date is provided in dataset or proposal filters to avoid empty query results.
Bug Fixes:
Enhancements:
Tests: