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.

Sign up for GitHub

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

Jump to bottom

WC2-523: [WEB] NFC card field shows 0 for all beneficiaries even when card is assigned #1735

Merged
merged 8 commits into from
Oct 31, 2024
Merged

WC2-523: [WEB] NFC card field shows 0 for all beneficiaries even when card is assigned #1735

merged 8 commits into from
Oct 31, 2024

Conversation

hakifran
Copy link
Contributor

@hakifran hakifran commented Oct 23, 2024 •
edited
Loading

Explain what problem this PR is resolving

  • [WEB] NFC card field shows 0 for all beneficiaries even when card is assigned
    Related JIRA tickets : WC2-523

Self proofreading checklist

  • Did I use eslint and black formatters
  • Is my code clear enough and well documented
  • Are my typescript files well typed
  • New translations have been added or updated if new strings have been introduced in the frontend
  • My migrations file are included
  • Are there enough tests
  • Documentation has been included (for new feature)

Changes

  • Add nfc_cards count in the entity dict and serializer
  • Link the StorageLogEntry to the right entity if the import is from mobile(entity_uuid)

How to test

  • Create an instance linked to an entity
  • Create a StorageDevice linked to that entity with NFC type
  • Create one or multiple StorageLogEntry linked the entity, the instance and the created device
  • Check the nfc cards on the beneficiary details page and on the instance details page

Print Screen/video

Screencast.from.2024-10-23.11-18-46.webm

@hakifran hakifran requested a review from bramj October 23, 2024 11:52
bramj
bramj requested changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@bramj bramj left a comment

Choose a reason for hiding this comment

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

Nice! The counts work 👍 I have a few doubts about performance issues as the logs table grows, but I think it might be difficult to fix, see comments.

Main thing is I think there's something missing on this screen:
http://localhost:8081/dashboard/storages/
image
I did the setup as per instructions in the PR, but I see no beneficiary on the "General" or "Logs" entries. The status on the log is also missing (I set it to "OK" in my db).

iaso/api/storage.py
concerned_entity = Entity.objects.get(uuid=log_data["entity_id"])
entity_id = log_data.get("entity_id") or log_data.get("entity_uuid")
if entity_id:
concerned_entity = Entity.objects.get(uuid=entity_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

iaso/api/entity.py
nfc_log_entries_count = StorageLogEntry.objects.filter(device__in=nfc_devices).count()

return nfc_log_entries_count

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be careful to add this extra query on the EntitySerializer. I also expect the amount of records in StorageLogEntry to grow.
Did you try to do it with a relation and prefetch_related? If it's complicated then it's okay for me to go with it like this.

@hakifran
Copy link
Contributor Author

hakifran commented Oct 24, 2024 •
edited
Loading

Nice! The counts work 👍 I have a few doubts about performance issues as the logs table grows, but I think it might be difficult to fix, see comments.

Main thing is I think there's something missing on this screen: http://localhost:8081/dashboard/storages/ image I did the setup as per instructions in the PR, but I see no beneficiary on the "General" or "Logs" entries. The status on the log is also missing (I set it to "OK" in my db).

Did you link the storageDevice with the entity?
Screenshot from 2024-10-24 17-03-30

The logs status, status_reason and status_comment must also be save in storageLogEntry
Screenshot from 2024-10-24 17-21-37

@bramj
Copy link
Contributor

bramj commented Oct 25, 2024

@hakifran Yes I think it's a misalignment between the returned JSON and the frontend

image

Btw, I see the name is blank on the Entity, but in reality this will almost always be the case. So better to just display the id in the frontend.

@hakifran hakifran requested a review from bramj October 25, 2024 09:35
@beygorghor beygorghor added the release Should be released in production at next deploy label Oct 30, 2024
bramj
bramj approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@bramj bramj left a comment

Choose a reason for hiding this comment

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

Tested again and this looks very good to me 👍

iaso/api/entity.py
def get_nfc_cards(self, entity: Entity):
nfc_log_entries_count = StorageDevice.objects.filter(entity=entity, type=StorageDevice.NFC).aggregate(
total_logs=Count("log_entries")
)["total_logs"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Looking good to me 👍

@hakifran hakifran merged commit 97835ca into main Oct 31, 2024
3 checks passed
@hakifran hakifran deleted the WC2-523-NFC-card-field-shows-0-for-all-beneficiaries branch October 31, 2024 13:08
@quang-le quang-le added user tested Has already been tested on staging Released and removed release Should be released in production at next deploy user tested Has already been tested on staging labels Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bramj bramj bramj approved these changes

Assignees
No one assigned
Labels
Released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hakifran @bramj @beygorghor @quang-le