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

IA-3776 Add OU change requests to bulk upload #1859

Draft
wants to merge 1 commit into
base: WC2-523-storage-logs-bulk-update
Choose a base branch
from

Conversation

bramj
Copy link
Contributor

@bramj bramj commented Dec 10, 2024

What problem is this PR solving? Explain here in one sentence.

Related JIRA tickets : IA-XXX, WC2-XXX, POLIO-XXX

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)

Doc

Tell us where the doc can be found (docs folder, wiki, in the code...).

Changes

Explain the changes that were made.

The idea is not to list exhaustively all the changes made (GitHub already provides a full diff), but to help the reviewers better understand:

  • which specific file changes go together, e.g: when creating a table in the front-end, there usually is a config file that goes with it
  • the reasoning behind some changes, e.g: deleted files because they are now redundant
  • the behaviour to expect, e.g: tooltip has purple background color because the client likes it so, changed a key in the API response to be consistent with other endpoints

How to test

Explain how to test your PR.

If a specific config is required explain it here: dataset, account, profile, etc.

Print screen / video

Upload here print screens or videos showing the changes.

Notes

Things that the reviewers should know:

  • known bugs that are out of the scope of the PR
  • other trade-offs that were made
  • does the PR depends on a PR in bluesquare-components?
  • should the PR be merged into another PR?

Follow the Conventional Commits specification

The merge message of a pull request must follow the Conventional Commits specification.

This convention helps to automatically generate release notes.

Use lowercase for consistency.

Example:

fix: empty instance pop up

Refs: IA-3665

Note that the Jira reference is preceded by a line break.

Both the line break and the Jira reference are entered in the Add an optional extended description… field.

@bramj
Copy link
Contributor Author

bramj commented Dec 10, 2024

@bmonjoie I'm a bit stuck on this one to be honest 🤔
When I receive an OU change requests for which the user has no permission (e.g. geo limitation), I don't want to make the whole upload fail. On the other hand, I want the bulk upload to stay atomic.

With the current implementation, what happens in this scenario? The API endpoint would return an error, wouldn't the user be blocked by this on the mobile app?

@bmonjoie
Copy link
Member

@bramj I think they would be, but they shouldn't, just like with Storage Logs, Instances, or OrgUnits.

@bramj
Copy link
Contributor Author

bramj commented Dec 12, 2024

@bmonjoie I agree we shouldn't block users because of this. And in fact with the bulk upload they wouldn't be, so that's an improvement. However I fear such a scenario will result in the user seeing "all is fine" and the backend throwing a Sentry (which we may not pay attention to) and ignoring the incorrect data. In the end the user might end up wondering why data doesn't arrive.

I think as a solution for now I'll make sure the bulk upload does not fail, but have it return some feedback about this sort of stuff in the task's completion message.

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