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

Adds edit, create and delete views #364

Merged
merged 15 commits into from
Oct 4, 2024
Merged

Adds edit, create and delete views #364

merged 15 commits into from
Oct 4, 2024

Conversation

dalonsoa
Copy link
Collaborator

@dalonsoa dalonsoa commented Oct 2, 2024

Both, generic that will be used in other apps, and the concrete implementation for the data import app. Now this view is linked from the menu bar and can be accessed and used by any logged user.

When implementing the edit and new views, that need to handle the ingestion of data files, a couple of extra bits of the code had to be updated to ensure that things behaved as they should.

This PR doesn't adress #334 , as enabling that is more convoluted that just something done in the views.

Close #343

@dalonsoa dalonsoa requested review from Sahil590 and tsmbland October 2, 2024 12:41
@dalonsoa dalonsoa linked an issue Oct 3, 2024 that may be closed by this pull request
Copy link
Member

@tsmbland tsmbland left a comment

Choose a reason for hiding this comment

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

I've only had a quick glance at the code, but I've tried running it and it seems to work, with the following caveats/questions:

  • It's still not really clear what format the data is supposed to be in. What columns need to be in the file? It's not obvious from the documentation, unless I'm missing something. And all the options for the Format field are in Spanish.

  • I'm a bit confused by the "Reprocess data" checkbox. I guess it's so you can re-trigger the import if something fails, but wouldn't this be better as a button?

  • The submission date is off by an hour. I guess it's showing UTC, but I think it would be more useful to show in the local timezone

  • The screenshot in the documentation shows an Owner field which is no longer in the form. I guess the person performing the import automatically becomes the owner, so no need for it in the form, but when I go to view the import object after creating it the Owner field appears empty.

  • Does deleting the import delete the measurements from the database or just the import object?

@dalonsoa
Copy link
Collaborator Author

dalonsoa commented Oct 3, 2024

Many thanks for the feedback. This is really useful.

A few answers:

  • It's still not really clear what format the data is supposed to be in. What columns need to be in the file? It's not obvious from the documentation, unless I'm missing something. And all the options for the Format field are in Spanish.

It is unclear because the underlying design of the models I think is still a mess. You choose the format, but then the data processing is done by the classifications, which is something happening under the hood. The whole thing needs more clarity, and not just the documentation, but the actual workflow and the options presented to the user. I'll open an issue about it. --> Done #373

  • I'm a bit confused by the "Reprocess data" checkbox. I guess it's so you can re-trigger the import if something fails, but wouldn't this be better as a button?

This was implemented when we only had the Admin interface to play with, so the re-process checkbox was the best (only) choice. Now that we have our own custom views, I agree that a button will work much better. I'll open another issue about it. --> Done #374

  • The submission date is off by an hour. I guess it's showing UTC, but I think it would be more useful to show in the local timezone

I hate timezones. I'll check the best way forward, because I think there's something iffy with how time data is presented in the front end. I'll try to tackle this here, and if it seems deeper, I will open an issue.

  • The screenshot in the documentation shows an Owner field which is no longer in the form. I guess the person performing the import automatically becomes the owner, so no need for it in the form, but when I go to view the import object after creating it the Owner field appears empty.

Mmmm... That's annoying. It should have been assigned automatically. I'll fix it.

  • Does deleting the import delete the measurements from the database or just the import object?

No, it does not delete the associated measurements. We have an issue about that, #338 and my comment therein. I was hopping to connect data import and measurements this week, but I'm not sure if I'll have time.

@dalonsoa
Copy link
Collaborator Author

dalonsoa commented Oct 3, 2024

@tsmbland , owner issue just fixed in 14f3123

@dalonsoa
Copy link
Collaborator Author

dalonsoa commented Oct 4, 2024

@tsmbland , I've been checking the timezone issue and it is something that needs fixing across the whole of Paricia to make sense - also in the dash apps - so I'll open an issue and fix things there.

For reference, some times (like the submission time) should be presented to the user in the user's timezone, but others (like the data being ingested, or the plots in the dash apps) should be presented to the user in the original timezone the data belongs to. At the moment, everything is presented in UTC, which is the default timezone of the app, as indicated in the settings. This is confusing. More information, in https://docs.djangoproject.com/en/5.1/topics/i18n/timezones/

Issue open --> #375

@dalonsoa dalonsoa merged commit 46041f8 into main Oct 4, 2024
7 checks passed
@dalonsoa dalonsoa deleted the edit_view branch October 4, 2024 05:26
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.

Add views to the importing app Extract data import as a separate view, not using the admin
2 participants