Skip to content
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

1432: transfers grid #2451

Merged
merged 19 commits into from
Nov 20, 2024
Merged

1432: transfers grid #2451

merged 19 commits into from
Nov 20, 2024

Conversation

BCerki
Copy link
Contributor

@BCerki BCerki commented Nov 6, 2024

card: https://github.com/orgs/bcgov/projects/123/views/16?pane=issue&itemId=80445864&issue=bcgov%7Ccas-registration%7C1432

This PR:

  • creates all the pages and components for the transfer grid for both cas_analyst and cas_admin
  • the transfer service returns the data in the correct shape for the grid (facilities each have their own row even though in the db it's a m2m field). Otherwise, the back end work for this grid is pretty much the same as any of the grids
  • the typing was tricky because once we grab values() from a queryset it ceases to become a queryset (as far as mypy is concerned; it seems to behave the same as far as I can tell)
  • cleaned up the transfer model (updated statuses and removed a old field)
  • pytests and vitests

@BCerki BCerki force-pushed the 1432-transfers-grid branch 7 times, most recently from 4b7af90 to 2780368 Compare November 13, 2024 18:19
@@ -29,4 +29,5 @@
USER_OPERATOR_TAGS_V2 = ["User Operator V2"]
MISC_TAGS = ["Misc V1"]
CONTACT_TAGS = ["Contact V1"]
TRANSFER_EVENT_TAGS = ["Transfer Event V1"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct--even though we're in reg2, we didn't have events in reg2, so I modelled this after facility

facility_id = getattr(obj, 'facilities__id', None)

record_id = operation_id if operation_id else facility_id
if not isinstance(record_id, UUID):
Copy link
Contributor Author

@BCerki BCerki Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make mypy happy but also probably a good check

sort_field: Optional[str],
sort_order: Optional[str],
filters: TransferEventFilterSchema = Query(...),
) -> List[Dict[str, Any]]:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy said that you can't have a QuerySet[List], it has to be QuerySet[ModelName], so I've gone with this

}) => {
return {
id,
operation__name: operation__name || "N/A",
Copy link
Contributor Author

@BCerki BCerki Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filtering by N/A doesn't work--I'll work on it once I get my computer charged again, possibly in a different PR

@@ -0,0 +1,24 @@
const formatTimestamp = (timestamp: string) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a copy over from reg1, not a refactor. I left reg1 using its own version of this function

@BCerki BCerki marked this pull request as ready for review November 14, 2024 17:10
@BCerki BCerki force-pushed the 1432-transfers-grid branch from 4fe3a3a to b484f90 Compare November 15, 2024 23:16
Copy link
Contributor

@Sepehr-Sobhani Sepehr-Sobhani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! 🤩 I've added a few minor comments, but nothing critical.

Additionally, I think we need to update the content of the Transfers tile in both common/fixtures/dashboard/registration/internal.json and common/fixtures/dashboard/registration/internal_admin.json.

Screenshot 2024-11-19 at 4 22 20 PM

Comment on lines 12 to 15
class FacilityForTransferEventGrid(ModelSchema):
class Meta:
model = Facility
fields = ['name']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover!

Comment on lines 49 to 46
def filtering_including_not_applicable(self, field: str, value: str) -> Q:
if value and re.search(value, 'n/a', re.IGNORECASE):
return Q(**{f"{field}__icontains": value}) | Q(**{f"{field}__isnull": True})
return Q(**{f"{field}__icontains": value}) if value else Q()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this 😍 nit: just needs @staticmethod decorator since we are not using the self inside the function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend returning the QuerySet from this service and allowing the ninja router to transform the QuerySet into a list. We can also satisfy mypy by casting the result as shown below:

from typing import cast
from django.db.models import QuerySet
from registration.models.event.transfer_event import TransferEvent
from typing import Optional
from registration.schema.v1.transfer_event import TransferEventFilterSchema
from ninja import Query


class TransferEventService:
    @classmethod
    def list_transfer_events(
        cls,
        sort_field: Optional[str],
        sort_order: Optional[str],
        filters: TransferEventFilterSchema = Query(...),
    ) -> QuerySet[TransferEvent]:
        sort_direction = "-" if sort_order == "desc" else ""
        sort_by = f"{sort_direction}{sort_field}"
        queryset = (
            filters.filter(TransferEvent.objects.order_by(sort_by))
            .values(
                'effective_date',
                'status',
                'created_at',
                'operation__name',
                'operation__id',
                'facilities__name',
                'facilities__id',
            )
            .distinct()
        )
        return cast(QuerySet[TransferEvent], queryset)

Comment on lines 18 to 20
if (!transfers) {
return <div>No transfers data in database.</div>;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we throw an error if !transfers || "error" in transfers? This condition indicates that something went wrong on the backend. If that's the case, we should display an error. Otherwise, we can show an empty data grid, similar to when there is simply no data to display. This approach helps us distinguish between an actual error and the absence of data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is setup the same way as the other grids, I'll fix them too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We started throwing errors in the registration forms at some point, but those grids are from before, I think. So don’t worry if it’s too much work; we can address it as technical debt.

{
field: "created_at",
headerName: "Submission Date",
// Set flex to 1 to make the column take up all the remaining width if user zooms out
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should create this page for cas_analyst as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the latest role miro board, it's now only cas_analyst that will be allowed to do transfers, so I've moved this

Comment on lines +82 to +83
today = timezone.make_aware(datetime.now())
yesterday = today - timedelta(days=1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More lovely codes 😍

row_count: number;
};
}) => {
console.log("initial", initialData);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover!

@BCerki
Copy link
Contributor Author

BCerki commented Nov 20, 2024

filtering_including_not_applicable

Great work! 🤩 I've added a few minor comments, but nothing critical.

Additionally, I think we need to update the content of the Transfers tile in both common/fixtures/dashboard/registration/internal.json and common/fixtures/dashboard/registration/internal_admin.json.

Screenshot 2024-11-19 at 4 22 20 PM

I'm going to make a separate ticket for this; when I talked to Zoey about this it sounds like we have some more general work to do on the dashboards

@BCerki BCerki force-pushed the 1432-transfers-grid branch from 7470296 to 7be260a Compare November 20, 2024 22:53
@BCerki BCerki merged commit 1e96823 into develop Nov 20, 2024
40 checks passed
@BCerki BCerki deleted the 1432-transfers-grid branch November 20, 2024 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants