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

Form-Data and multi-part request feature added #101

Merged
merged 40 commits into from
Jan 7, 2024

Conversation

vidya-hub
Copy link
Contributor

@vidya-hub vidya-hub commented Dec 20, 2023

**Multi-Part Request Feature #92,#91 **

  • Added form-data option in request body content type.
  • now we can select text or file type in form-data fields and send multi-part Request.
  • Form-Data can be Saved in Local DB and can Load from the saved data.
  • Added multipart Code generation feature for all supported languages in the Code Gen DropDown

lib/consts.dart Outdated
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:google_fonts/google_fonts.dart';
import 'package:davi/davi.dart';

Copy link
Member

Choose a reason for hiding this comment

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

Feedback in regards to import across all the files.
Please revert all import re-arrangements that have been done as they are introducing non-essential code changes.
Only add new import lines or modify existing lines.

lib/consts.dart Outdated
@@ -294,6 +298,15 @@ const kSubTypeDefaultViewOptions = 'all';
const kContentTypeMap = {
ContentType.json: "$kTypeApplication/$kSubTypeJson",
ContentType.text: "$kTypeText/$kSubTypePlain",
ContentType.formdata: "multipart/form-data",
};
const kFormDataTypeMap = {
Copy link
Member

Choose a reason for hiding this comment

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

kFormDataTypeMap & kMapFormDataType can be avoided if FormDataType is structured as CodegenLanguage

import '../models/models.dart';
import 'dart:typed_data';

import 'package:apidash/models/form_data_model.dart';
Copy link
Member

Choose a reason for hiding this comment

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

it should already be imported via '../models/models.dart'

@@ -63,3 +65,59 @@ Future<(http.Response?, Duration?, String?)> request(
return (null, null, uriRec.$2);
}
}

Future<(http.Response?, Duration?, String?)> multiPartRequest(
Copy link
Member

Choose a reason for hiding this comment

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

Can the existing function be modified to support multipart using a multipart = true argument

@ashitaprasad
Copy link
Member

Thanks for the PR @vidya-hub
Apart from the comments mentioned above. PFA a few more observations.

Screenshot 2023-12-22 at 9 23 12 AM

There are overflow issues. It should be along the lines of params and headers which have an empty default row in the beginning and the user is able to clear the first row (which gets replaced by an empty row) if it is the only row. Please play around with headers & params to better understand the behaviour.

Also, the color scheme of the file picker widget (purple) is not following the color theme of API Dash.

@ashitaprasad ashitaprasad added work in progress Dev team is already working on this issue and removed under review labels Dec 22, 2023
@vidya-hub
Copy link
Contributor Author

Hi @ashitaprasad

Thanks for your comments and observations.
Sure I will work on those things and once it is done I will push the code changes.

Thank you.

lib/consts.dart Outdated
@@ -48,6 +48,10 @@ const kHintOpacity = 0.6;
const kForegroundOpacity = 0.05;

const kTextStyleButton = TextStyle(fontWeight: FontWeight.bold);
const kFormDataButton = TextStyle(
Copy link
Member

Choose a reason for hiding this comment

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

kFormDataButton -> kFormDataButtonLabelTextStyle

@ashitaprasad
Copy link
Member

Hi @vidya-hub
I have reviewed the PR. There are still pending issues that you need to fix that I pointed out in the earlier review.

@vidya-hub
Copy link
Contributor Author

Hey @ashitaprasad

Thank you

Can you tell me which issues I've missed so that i can give a fix for that

@ashitaprasad
Copy link
Member

Sure @vidya-hub

This comment and this comment

(http.Response?, Duration?, String?)? responseRec = await request(
requestModel,
defaultUriScheme: defaultUriScheme,
isMultiPartRequest: requestModel.formDataList != null &&
Copy link
Member

Choose a reason for hiding this comment

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

requestBodyContentType is the correct way to determine the value of isMultiPartRequest.

@@ -108,36 +108,40 @@ class _RequestListState extends ConsumerState<RequestList> {
controller: controller,
Copy link
Member

Choose a reason for hiding this comment

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

Revert this change which has nothing to do with formdata & we already show a full page message in details pane when there is no request.

@@ -3,6 +3,7 @@ import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:apidash/providers/providers.dart';
import 'package:apidash/widgets/widgets.dart';
import 'package:apidash/consts.dart';
import 'package:apidash/widgets/form_data_widget.dart';
Copy link
Member

Choose a reason for hiding this comment

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

Add form_data_widget.dart to widgets.dart which is already imported.

@@ -12,12 +13,21 @@ class EditRequestBody extends ConsumerStatefulWidget {
}

class _EditRequestBodyState extends ConsumerState<EditRequestBody> {
@override
Copy link
Member

Choose a reason for hiding this comment

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

Remove as you are not doing anything inside initState()

@ashitaprasad
Copy link
Member

@vidya-hub I have made some changes to the PR.
Can you go through it once and test it out.

@vidya-hub
Copy link
Contributor Author

Sure @ashitaprasad

@vidya-hub
Copy link
Contributor Author

vidya-hub commented Jan 7, 2024

@ashitaprasad

I've gone through the changes and tested it everything is working fine in form-data side.
Thanks.

@ashitaprasad
Copy link
Member

Hi @vidya-hub,
I have fixed some tests.
But, currently js & nodejs codegen tests are failing. This might be because of the lack of check in the codegens for the presence of any form data.
Kindly, run flutter test to see the detailed test results and kindly fix the codegens.
Thanks!

@vidya-hub
Copy link
Contributor Author

Thank you @ashitaprasad

I will check and update.

@vidya-hub
Copy link
Contributor Author

image

Hi @ashitaprasad
I've fixed all flutter-test-related issues and all tests are passed can you please check once.

Thanks!

@ashitaprasad
Copy link
Member

Sure @vidya-hub 👍

@ashitaprasad
Copy link
Member

ashitaprasad commented Jan 7, 2024

@vidya-hub Since you are not adding any new test cases for multipart forms, please make changes only to the codegen codes and revert any changes made to the expected codes (even spacings) given in the test files.
These expected codes are golden tests that have already been run, verified and formatted properly.

@vidya-hub
Copy link
Contributor Author

Sure @ashitaprasad

@vidya-hub
Copy link
Contributor Author

Hi @ashitaprasad

test/models/request_model_test.dart

can you please check this file in PR once

@ashitaprasad
Copy link
Member

@vidya-hub test/models/request_model_test.dart will only change as the model has been changed.
I was talking about the codegen test cases that contain the expected code.

rev: reverted changes in js code gen
@vidya-hub
Copy link
Contributor Author

Thank you,
Understood @ashitaprasad.

I've pushed the changes,
can you please check once .

@ashitaprasad
Copy link
Member

Sure @vidya-hub 👍

@ashitaprasad
Copy link
Member

@vidya-hub There was a minor issue with python codegen which I have fixed.
LGTM. It is ready for merge 🚀
The only thing remaining is adding codegen tests for this feature which we can add in a separate PR once I open an issue & assign it to you after figuring out a good public endpoint.
Thanks for the awesome PR 🙌

@ashitaprasad ashitaprasad merged commit 252b014 into foss42:main Jan 7, 2024
@vidya-hub
Copy link
Contributor Author

Thanks a lot @ashitaprasad

Sure I am working on it already.

Thank you for your comments and suggestions.
I've learnt a lot from you by implementing this feature.

@ashitaprasad
Copy link
Member

@vidya-hub
It was a blast working with you on this valuable PR! 🎉
Definitely looking forward to more contributions from you 🤘
Don't forget to share this wonderful open source achievement on your socials (Twitter, LinkedIn). 🥳

@vidya-hub
Copy link
Contributor Author

Thank you @ashitaprasad.

Sure 😊

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.

3 participants